feat: add AES-GCM encrypted file fallback for keychain storage#146
feat: add AES-GCM encrypted file fallback for keychain storage#146sunxiaoguang wants to merge 4 commits intolarksuite:mainfrom
Conversation
This commit adds a locally managed encrypted store to serve as a fallback when the system keychain is unreachable. - Centralizes AES-GCM encryption logic into internal/keychain. - Updates auth token and app secret resolution to support fallback files. - Adds user warnings when falling back to local encrypted storage. - Improves 'config init' reliability with better validation and rollbacks. - Refactors config directory handling into a dedicated internal package. Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>
|
|
📝 WalkthroughWalkthroughCentralizes config-dir resolution; adds AES‑GCM encrypted-file fallback for keychain unavailability; extends secret/token storage to prefer system keychain then fall back to encrypted files with validation, rollback, and user warnings; refactors config-init to use new storage/validation helpers. (50 words) Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI App
participant SysKC as System Keychain
participant EncFS as Encrypted File Fallback
participant Config as Config Store
User->>CLI: config init / login (store secret/token)
CLI->>SysKC: Attempt Set (keychain)
alt SysKC Unavailable (ShouldUseFallback)
SysKC-->>CLI: ErrUnavailable
CLI->>EncFS: SetFallback (AES‑GCM file + master.key)
EncFS-->>CLI: OK (encrypted file written)
CLI->>Config: Save config with SecretRef {source:"encrypted_file"}
Config-->>CLI: OK
CLI->>User: Emit encrypted-fallback warning (stderr)
else SysKC OK
SysKC-->>CLI: OK (keychain stored)
CLI->>Config: Save config with SecretRef {source:"keychain"}
Config-->>CLI: OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds an AES-GCM encrypted, CLI-managed file store as a fallback for credentials when the macOS native keychain is unavailable (e.g., sandbox restrictions), and updates config/token workflows to use it safely.
Changes:
- Introduces a shared encrypted-file storage layer with a file-based fallback directory and typed “keychain unavailable” error signaling.
- Refactors secret storage/resolution to support a new
encrypted_fileSecretRef source, plus rollback + validation inconfig init. - Migrates token storage to use the fallback store while retaining read-compatibility with legacy unencrypted token files.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/keychain/keychain_other.go | Removes duplicated crypto/file I/O and delegates to shared encrypted-file helpers on Linux. |
| internal/keychain/keychain_darwin.go | Wraps keychain master-key failures as “unavailable” to enable fallback; delegates file I/O to shared helpers. |
| internal/keychain/file_encrypted_store.go | Adds centralized AES-GCM + master.key handling + fallback Get/Set/Remove APIs. |
| internal/keychain/file_encrypted_store_test.go | Tests encryption, permissions, and cleanup behavior for fallback store. |
| internal/keychain/fallback_policy.go | Adds typed policy (ShouldUseFallback) for deciding when to degrade. |
| internal/keychain/fallback_policy_test.go | Tests fallback policy only triggers on typed unavailable errors. |
| internal/keychain/errors.go | Introduces ErrUnavailable + WrapUnavailable to standardize fallback-eligible failures. |
| internal/core/secret.go | Extends SecretRef schema to allow encrypted_file source. |
| internal/core/secret_resolve.go | Adds encrypted fallback secret resolution + new storage helper with fallback; updates removal logic. |
| internal/core/secret_resolve_test.go | Adds tests for secret storage behavior and file-based secret resolution. |
| internal/core/config.go | Centralizes config dir resolution via new internal/configdir package. |
| internal/configdir/config_dir.go | New package for resolving CLI config dir (env override + home fallback). |
| internal/auth/token_store.go | Adds encrypted fallback + legacy token file migration and cleanup. |
| internal/auth/token_store_test.go | Tests fallback token persistence, removal, legacy reads, and non-fallback on generic errors. |
| cmd/doctor/doctor.go | Updates doctor messaging to reflect “local credential store” rather than keychain only. |
| cmd/config/init.go | Refactors init storage pipeline, adds rollback on save failure, adds secret-ref reuse validation, and warns on fallback usage. |
| cmd/config/init_storage_test.go | Adds tests for fallback triggering, rollback behavior, and secret reuse validation. |
| cmd/config/init_command_test.go | Moves/clarifies test responsibilities between command wiring vs storage behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key = make([]byte, masterKeyBytes) | ||
| if _, err := rand.Read(key); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := validate.AtomicWrite(keyPath, key, 0600); err != nil { | ||
| if existingKey, readErr := loadMasterKeyFile(dir); readErr == nil { | ||
| return existingKey, nil | ||
| } | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
loadOrCreateMasterKeyFile uses validate.AtomicWrite to create master.key after a read miss. AtomicWrite renames over the target path unconditionally (see internal/validate/atomicwrite.go), so concurrent processes can race and overwrite an already-created master.key with a different value. That can permanently orphan any secrets written with the previous key. Consider creating master.key with an exclusive create (O_CREATE|O_EXCL) or a “rename-no-replace” strategy (or a filesystem lock) so the key is never replaced once created; if creation fails because the file exists, re-read and use the existing key.
| var safeFileNameRe = regexp.MustCompile(`[^a-zA-Z0-9._-]`) | ||
|
|
||
| func safeFileName(account string) string { | ||
| return safeFileNameRe.ReplaceAllString(account, "_") + ".enc" | ||
| } |
There was a problem hiding this comment.
safeFileName() replaces all non-[a-zA-Z0-9.-] characters with "", which can cause different account IDs to collide to the same filename (e.g., ":" vs "_" or other replaced chars), leading to silent overwrites and wrong secret/token retrieval. To make filenames collision-free, consider encoding the full account string (e.g., base64.RawURLEncoding or hex(SHA-256(account))) instead of lossy character replacement.
|
|
||
| select { | ||
| case res := <-resCh: | ||
| return res.key, res.err | ||
| if res.err != nil { | ||
| return nil, WrapUnavailable(res.err) | ||
| } | ||
| return res.key, nil | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| } | ||
| } | ||
|
|
||
| func encryptData(plaintext string, key []byte) ([]byte, error) { | ||
| block, err := aes.NewCipher(key) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| aesGCM, err := cipher.NewGCM(block) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| iv := make([]byte, ivBytes) | ||
| if _, err := rand.Read(iv); err != nil { | ||
| return nil, err | ||
| return nil, WrapUnavailable(ctx.Err()) |
There was a problem hiding this comment.
The new WrapUnavailable handling makes keychain failures fallback-eligible, but getMasterKey() still generates a brand-new master key whenever keyring.Get(service, "master.key") returns any error (not just “not found”). If keyring.Get fails transiently (locked/denied read) but keyring.Set succeeds, this rotates master.key and permanently breaks decryption of existing encrypted files. Consider only generating a new master key when the error is a definite not-found (e.g., errors.Is(err, keyring.ErrNotFound)) and otherwise returning WrapUnavailable(err) so callers can fall back without destroying existing data.
Greptile SummaryThis PR adds an AES-GCM encrypted file fallback for credential storage so Key changes:
One logic gap found: In Confidence Score: 4/5Safe to merge with one minor logic gap in the legacy-fallback routing that is unlikely to surface in practice. The PR is well-structured with strong test coverage across all new code paths. The single remaining finding is a P2 logical inconsistency in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SetStoredToken / ForStorageWithEncryptedFallback] --> B{Primary keychain Set succeeds?}
B -- yes --> C[Remove encrypted fallback file\nRemove legacy file\nreturn usedFallback=false]
B -- no --> D{ShouldUseFallback?\nerrors.Is ErrUnavailable}
D -- no --> E[Return keychain error]
D -- yes --> F[SetFallback — AES-GCM\n~/.lark-cli/keychain/svc/]
F --> G[Remove legacy file\nreturn usedFallback=true]
H[GetStoredToken] --> I{Primary keychain Get non-empty?}
I -- yes, valid JSON --> J[Return token]
I -- no / error --> K[GetFallbackWithError]
K -- success --> L[Parse JSON and return]
K -- error --> M{os.IsNotExist?}
M -- yes --> N[readLegacyManagedToken\nlegacy .json file]
M -- no --> O[Return nil / re-auth required]
style F fill:#ffe0b2
style G fill:#ffe0b2
style N fill:#fff9c4
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (1)
cmd/config/init_storage_test.go (1)
140-145: Add platform guard or use platform-neutral failure injection.The test at line 142 uses
os.Chmod(configDir, 0500)to simulate a write failure. On Windows,os.Chmoddoes not enforce POSIX write semantics on directories, making this mechanism unreliable across platforms. While Windows is not currently in the CI test matrix, the test should either be guarded with//go:build !windowsto make the platform limitation explicit, or refactored to use a platform-neutral failure injection (e.g., mocking the underlying save operation directly rather than relying on filesystem permissions).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init_storage_test.go` around lines 140 - 145, The test uses os.Chmod(configDir, 0500) inside the trackingKeychain.setFunc to induce a write failure, which is unreliable on Windows; either add a platform build constraint (e.g., add //go:build !windows to this test file) to skip it on Windows, or refactor the test to inject a platform-neutral failure by mocking the save path instead of changing filesystem permissions — for example modify the test harness around trackingKeychain and its setFunc to return a synthetic error (e.g., return fmt.Errorf("simulated write error")) when invoked, ensuring the test asserts that the code path in the init/storage logic handles that error without relying on os.Chmod.
🤖 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/secret_resolve.go`:
- Around line 61-77: ForStorageWithEncryptedFallback currently leaves the other
backend's secret when switching backends; update it so after a successful kc.Set
(in ForStorageWithEncryptedFallback) you also remove the encrypted-file fallback
entry for secretAccountKey(appId) (call the inverse of keychain.SetFallback,
e.g., keychain.DeleteFallback(keychain.LarkCliService, key) or
keychain.RemoveFallback) and likewise after a successful keychain.SetFallback
call remove any existing keychain entry for the same key via the KeychainAccess
delete method (kc.Delete or similar); perform deletions best-effort (ignore
non-fatal errors but return the primary store error if that failed) so the ref
source no longer leaves a stale secret in the other backend.
In `@internal/keychain/errors.go`:
- Around line 16-20: WrapUnavailable currently loses the original error from the
unwrap chain; change its return to join ErrUnavailable with the original err so
callers can inspect both (use errors.Join(ErrUnavailable, err) or otherwise
combine errors so both are unwrappable) — update the WrapUnavailable function to
return errors.Join(ErrUnavailable, err) when err is non-nil and not already
ErrUnavailable, referring to the WrapUnavailable function and ErrUnavailable
symbol.
In `@internal/keychain/file_encrypted_store.go`:
- Around line 45-62: loadOrCreateMasterKeyFile currently writes the generated
master.key into the same fallback directory as the ciphertext (via
validate.AtomicWrite and loadMasterKeyFile), which defeats encryption if the
directory is readable; change the design so the AES master key is not stored
next to the blobs: either integrate with the OS secure key store / keyring API
(replace loadMasterKeyFile calls with a keyring-backed getter), or derive the
master key from a user secret (e.g., scrypt/Argon2 with a stored salt) and only
store non-secret salt next to blobs; update loadOrCreateMasterKeyFile,
loadMasterKeyFile and any validate.AtomicWrite usage to instead read/write the
key from the secure location or perform derivation, and ensure fallback/write
permissions and error handling remain intact.
- Around line 45-67: loadOrCreateMasterKeyFile must only create a new master.key
when the file truly does not exist; currently it generates a key on any read
error and uses validate.AtomicWrite (which renames over the destination) causing
races and unintended rotations. Change loadOrCreateMasterKeyFile to first call
loadMasterKeyFile and if it returns a key return it; if it returns an error,
check if the error is os.IsNotExist (only then proceed to create dir and
generate a new key); create the file using an exclusive-create strategy (e.g.,
write to a temp file and use os.OpenFile with os.O_CREATE|os.O_EXCL or attempt
to write and on EEXIST re-read via loadMasterKeyFile) so concurrent writers
cannot silently overwrite each other; if any write fails because the file now
exists, reload with loadMasterKeyFile and return that key. Ensure references:
loadOrCreateMasterKeyFile, loadMasterKeyFile, validate.AtomicWrite, master.key,
masterKeyBytes.
- Around line 22-25: The current safeFileName function and safeFileNameRe
produce many-to-one collisions by replacing characters with "_"—replace this
with a deterministic, collision-resistant encoder: compute a safe digest (e.g.,
SHA-256) of the full account string and encode it as hex or URL-safe base64 (no
padding), then append ".enc"; keep the original replacement approach only as a
human-readable prefix if desired (e.g., prefix + "-" + digest) so filenames
remain filesystem-safe and unique; update safeFileName and remove reliance on
safeFileNameRe for uniqueness while ensuring the output uses only
filesystem-safe characters.
---
Nitpick comments:
In `@cmd/config/init_storage_test.go`:
- Around line 140-145: The test uses os.Chmod(configDir, 0500) inside the
trackingKeychain.setFunc to induce a write failure, which is unreliable on
Windows; either add a platform build constraint (e.g., add //go:build !windows
to this test file) to skip it on Windows, or refactor the test to inject a
platform-neutral failure by mocking the save path instead of changing filesystem
permissions — for example modify the test harness around trackingKeychain and
its setFunc to return a synthetic error (e.g., return fmt.Errorf("simulated
write error")) when invoked, ensuring the test asserts that the code path in the
init/storage logic handles that error without relying on os.Chmod.
🪄 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: 210a05c8-dd69-4246-9db9-53d533ac04a4
📒 Files selected for processing (18)
cmd/config/init.gocmd/config/init_command_test.gocmd/config/init_storage_test.gocmd/doctor/doctor.gointernal/auth/token_store.gointernal/auth/token_store_test.gointernal/configdir/config_dir.gointernal/core/config.gointernal/core/secret.gointernal/core/secret_resolve.gointernal/core/secret_resolve_test.gointernal/keychain/errors.gointernal/keychain/fallback_policy.gointernal/keychain/fallback_policy_test.gointernal/keychain/file_encrypted_store.gointernal/keychain/file_encrypted_store_test.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.go
| // ForStorageWithEncryptedFallback stores a plain secret in keychain when available, | ||
| // or falls back to the shared encrypted file store. | ||
| func ForStorageWithEncryptedFallback(appId string, input SecretInput, kc keychain.KeychainAccess) (SecretInput, error) { | ||
| if !input.IsPlain() { | ||
| return input, nil | ||
| } | ||
| key := secretAccountKey(appId) | ||
| if err := kc.Set(keychain.LarkCliService, key, input.Plain); err == nil { | ||
| return SecretInput{Ref: &SecretRef{Source: "keychain", ID: key}}, nil | ||
| } else if !keychain.ShouldUseFallback(err) { | ||
| return SecretInput{}, fmt.Errorf("store secret in keychain: %w", err) | ||
| } | ||
| if err := keychain.SetFallback(keychain.LarkCliService, key, input.Plain); err != nil { | ||
| return SecretInput{}, fmt.Errorf("store secret encrypted fallback: %w", err) | ||
| } | ||
| return SecretInput{Ref: &SecretRef{Source: "encrypted_file", ID: key}}, nil | ||
| } |
There was a problem hiding this comment.
Same-app reinit can leave a stale encrypted_file secret on disk.
Once this helper starts returning a "keychain" ref again, the old fallback entry for the same appsecret:<appId> is never removed. cleanupOldConfig skips the current app in cmd/config/init.go:71-84, so rerunning config init after keychain access is restored leaves the secret in both backends. Please add a post-save cleanup path for the previous backend when the app ID stays the same and the ref source changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/secret_resolve.go` around lines 61 - 77,
ForStorageWithEncryptedFallback currently leaves the other backend's secret when
switching backends; update it so after a successful kc.Set (in
ForStorageWithEncryptedFallback) you also remove the encrypted-file fallback
entry for secretAccountKey(appId) (call the inverse of keychain.SetFallback,
e.g., keychain.DeleteFallback(keychain.LarkCliService, key) or
keychain.RemoveFallback) and likewise after a successful keychain.SetFallback
call remove any existing keychain entry for the same key via the KeychainAccess
delete method (kc.Delete or similar); perform deletions best-effort (ignore
non-fatal errors but return the primary store error if that failed) so the ref
source no longer leaves a stale secret in the other backend.
| func WrapUnavailable(err error) error { | ||
| if err == nil || errors.Is(err, ErrUnavailable) { | ||
| return err | ||
| } | ||
| return fmt.Errorf("%w: %v", ErrUnavailable, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the errors.go file to understand the current implementation
cat -n internal/keychain/errors.goRepository: larksuite/cli
Length of output: 777
🏁 Script executed:
# Check the keychain_darwin.go file to see how WrapUnavailable is used
fd -t f "keychain_darwin.go" | head -5 | xargs -I {} sh -c 'echo "File: {}"; cat -n {} | sed -n "60,75p"'Repository: larksuite/cli
Length of output: 531
🏁 Script executed:
# Let's verify the exact error wrapping semantics by checking Go documentation
# and testing fmt.Errorf behavior with %w
python3 << 'EOF'
# Document Go fmt.Errorf error wrapping semantics
# %w wraps the error (puts it in the error chain for errors.Unwrap)
# %v converts to string (does NOT wrap)
# In fmt.Errorf("%w: %v", ErrUnavailable, err):
# - ErrUnavailable is wrapped (position of %w)
# - err is formatted as string only (position of %v)
# This means:
# - errors.Is(result, ErrUnavailable) == true
# - errors.Is(result, originalErr) == false <-- This is the issue!
# - errors.Unwrap(result) returns ErrUnavailable, not originalErr
print("Go fmt.Errorf wrapping semantics:")
print("- %w: wraps the error (becomes part of error chain)")
print("- %v: formats as string only (not wrapped)")
print("")
print("Current implementation: fmt.Errorf(\"%w: %v\", ErrUnavailable, err)")
print("Result: Only ErrUnavailable is in the error chain")
print("Impact: errors.Is(err, originalError) will be false")
EOFRepository: larksuite/cli
Length of output: 354
Make underlying errors inspectable with errors.Is and errors.As.
The current implementation fmt.Errorf("%w: %v", ErrUnavailable, err) only wraps ErrUnavailable in the error chain. The original error (including context timeouts/cancellations at line 70) is lost from error chain inspection—callers cannot use errors.Is or errors.As to detect them. Use a pattern that wraps both the lower-level error and ErrUnavailable so the original cause remains unwrap-able.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/errors.go` around lines 16 - 20, WrapUnavailable currently
loses the original error from the unwrap chain; change its return to join
ErrUnavailable with the original err so callers can inspect both (use
errors.Join(ErrUnavailable, err) or otherwise combine errors so both are
unwrappable) — update the WrapUnavailable function to return
errors.Join(ErrUnavailable, err) when err is non-nil and not already
ErrUnavailable, referring to the WrapUnavailable function and ErrUnavailable
symbol.
| func loadOrCreateMasterKeyFile(dir string) ([]byte, error) { | ||
| keyPath := filepath.Join(dir, "master.key") | ||
|
|
||
| key, err := loadMasterKeyFile(dir) | ||
| if err == nil { | ||
| return key, nil | ||
| } | ||
|
|
||
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| key = make([]byte, masterKeyBytes) | ||
| if _, err := rand.Read(key); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := validate.AtomicWrite(keyPath, key, 0600); err != nil { | ||
| if existingKey, readErr := loadMasterKeyFile(dir); readErr == nil { |
There was a problem hiding this comment.
The fallback key is stored beside the ciphertext.
Writing master.key into the same fallback directory means any process or user that can read ~/.lark-cli/keychain/<service> can decrypt every entry. In practice the protection comes from the 0600 file mode, not the AES-GCM layer. If this downgrade is intentional, the warning/docs should say it is weaker than native keychain storage; otherwise the key has to come from something not stored next to the blobs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/file_encrypted_store.go` around lines 45 - 62,
loadOrCreateMasterKeyFile currently writes the generated master.key into the
same fallback directory as the ciphertext (via validate.AtomicWrite and
loadMasterKeyFile), which defeats encryption if the directory is readable;
change the design so the AES master key is not stored next to the blobs: either
integrate with the OS secure key store / keyring API (replace loadMasterKeyFile
calls with a keyring-backed getter), or derive the master key from a user secret
(e.g., scrypt/Argon2 with a stored salt) and only store non-secret salt next to
blobs; update loadOrCreateMasterKeyFile, loadMasterKeyFile and any
validate.AtomicWrite usage to instead read/write the key from the secure
location or perform derivation, and ensure fallback/write permissions and error
handling remain intact.
|
I will work on the comments later |
Signed-off-by: Xiaoguang Sun <sunxiaoguang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/auth/token_store.go (1)
69-89:⚠️ Potential issue | 🔴 CriticalHandle the “primary + fallback both exist” case explicitly.
A user can already have a token in the native keychain, then refresh/login while the keychain is unavailable and write a newer token only to fallback. Once the keychain is reachable again,
GetStoredTokenreturns the older primary copy first and ignores the newer fallback one, so auth can roll back to stale credentials. Please read both stores and resolve conflicts with a freshness signal that changes on refresh (for example later expiry metadata or a dedicated persisted write timestamp), then clean up the older copy. A regression test with stale-primary/newer-fallback coexistence would catch this.Also applies to: 92-109
cmd/auth/login.go (1)
294-312:⚠️ Potential issue | 🟠 MajorRoll back token mutations if saving
multifails.Both login paths persist the new token and delete other users' tokens before
core.SaveMultiAppConfig. If that save fails while switching users, the config still points at the old user set but some of those old tokens may already be gone, while the new token is orphaned. Please use the same store→save→rollback pattern here, or at least postpone old-token cleanup until after the config write succeeds.Also applies to: 376-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 294 - 312, When switching the active user you must avoid deleting other users' tokens before the config write succeeds: instead, capture the list of old userOpenIds from core.LoadMultiAppConfig, call core.SaveMultiAppConfig with the new app.Users first, and only after SaveMultiAppConfig returns nil iterate and call larkauth.RemoveStoredToken for the oldUser.UserOpenId values; if SaveMultiAppConfig fails, roll back the newly stored token by calling larkauth.RemoveStoredToken(config.AppID, openId) (the code paths using larkauth.SetStoredToken, larkauth.RemoveStoredToken, core.LoadMultiAppConfig and core.SaveMultiAppConfig should be updated accordingly).
🤖 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/keychain/keychain_darwin.go`:
- Around line 100-118: The Darwin read/remove paths now only use the new
safeFileName/raw-url-base64 filenames and will miss pre-upgrade items; update
the platform read function (the one that currently returns
readEncryptedFile(StorageDir(service), account, key)) and platformRemove to fall
back to the legacy Darwin filename mapping: after computing key via
getMasterKey(service) and attempting the normal
readEncryptedFile(StorageDir(service), account, key) (or removeEncryptedFile),
if that returns “not found”, try the legacy regexp-based filename variants used
by the old Darwin implementation (reconstruct the old filename mapping logic and
attempt readEncryptedFile/removeEncryptedFile for those legacy names under
StorageDir(service)); keep using readEncryptedFile, removeEncryptedFile,
safeFileName, getMasterKey and StorageDir so only lookup/migration logic
changes.
- Around line 37-58: The current logic in the getFn success branch incorrectly
wraps base64 decode and master-key length failures with WrapUnavailable, causing
corrupted key entries to be treated as "unavailable" and trigger fallbacks;
change the behavior in the branch where err == nil so that decode errors (from
base64.StdEncoding.DecodeString) and invalid length checks against
masterKeyBytes are returned as direct hard errors (e.g., fmt.Errorf or a
domain-specific error) instead of WrapUnavailable, while preserving
WrapUnavailable only for true keychain access failures (cases where
errors.Is(err, keyring.ErrNotFound) or other getFn errors); keep generation via
generateFn and storage via setFn unchanged.
---
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 294-312: When switching the active user you must avoid deleting
other users' tokens before the config write succeeds: instead, capture the list
of old userOpenIds from core.LoadMultiAppConfig, call core.SaveMultiAppConfig
with the new app.Users first, and only after SaveMultiAppConfig returns nil
iterate and call larkauth.RemoveStoredToken for the oldUser.UserOpenId values;
if SaveMultiAppConfig fails, roll back the newly stored token by calling
larkauth.RemoveStoredToken(config.AppID, openId) (the code paths using
larkauth.SetStoredToken, larkauth.RemoveStoredToken, core.LoadMultiAppConfig and
core.SaveMultiAppConfig should be updated accordingly).
🪄 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: 04b5fb62-cd6b-489e-bc56-c37cce15775f
📒 Files selected for processing (15)
cmd/auth/login.gocmd/auth/login_test.gocmd/config/init.gocmd/config/init_storage_test.gointernal/auth/token_store.gointernal/auth/token_store_test.gointernal/auth/uat_client.gointernal/core/secret_resolve.gointernal/core/secret_resolve_test.gointernal/keychain/errors.gointernal/keychain/fallback_policy_test.gointernal/keychain/file_encrypted_store.gointernal/keychain/file_encrypted_store_test.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/config/init_storage_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/keychain/fallback_policy_test.go
- internal/keychain/errors.go
- cmd/config/init.go
- internal/keychain/file_encrypted_store_test.go
- internal/core/secret_resolve_test.go
- internal/core/secret_resolve.go
| encodedKey, err := getFn() | ||
| switch { | ||
| case err == nil: | ||
| key, decodeErr := base64.StdEncoding.DecodeString(encodedKey) | ||
| if decodeErr != nil { | ||
| return nil, WrapUnavailable(fmt.Errorf("decode master key: %w", decodeErr)) | ||
| } | ||
| if len(key) != masterKeyBytes { | ||
| return nil, WrapUnavailable(fmt.Errorf("invalid master key length: %d", len(key))) | ||
| } | ||
| return key, nil | ||
| case errors.Is(err, keyring.ErrNotFound): | ||
| key, genErr := generateFn() | ||
| if genErr != nil { | ||
| return nil, genErr | ||
| } | ||
| if setErr := setFn(base64.StdEncoding.EncodeToString(key)); setErr != nil { | ||
| return nil, WrapUnavailable(setErr) | ||
| } | ||
| return key, nil | ||
| default: | ||
| return nil, WrapUnavailable(err) |
There was a problem hiding this comment.
Don't classify malformed master-key data as keychain unavailability.
ShouldUseFallback is supposed to trigger only for real keychain-unavailable failures. Wrapping base64/length validation errors with ErrUnavailable means a corrupted keyring entry will silently divert new writes into the fallback store, while the existing Darwin ciphertext remains unreadable. This should be a hard error instead of a fallback signal.
Possible fix
case err == nil:
key, decodeErr := base64.StdEncoding.DecodeString(encodedKey)
if decodeErr != nil {
- return nil, WrapUnavailable(fmt.Errorf("decode master key: %w", decodeErr))
+ return nil, fmt.Errorf("decode master key: %w", decodeErr)
}
if len(key) != masterKeyBytes {
- return nil, WrapUnavailable(fmt.Errorf("invalid master key length: %d", len(key)))
+ return nil, fmt.Errorf("invalid master key length: %d", len(key))
}
return key, nil📝 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.
| encodedKey, err := getFn() | |
| switch { | |
| case err == nil: | |
| key, decodeErr := base64.StdEncoding.DecodeString(encodedKey) | |
| if decodeErr != nil { | |
| return nil, WrapUnavailable(fmt.Errorf("decode master key: %w", decodeErr)) | |
| } | |
| if len(key) != masterKeyBytes { | |
| return nil, WrapUnavailable(fmt.Errorf("invalid master key length: %d", len(key))) | |
| } | |
| return key, nil | |
| case errors.Is(err, keyring.ErrNotFound): | |
| key, genErr := generateFn() | |
| if genErr != nil { | |
| return nil, genErr | |
| } | |
| if setErr := setFn(base64.StdEncoding.EncodeToString(key)); setErr != nil { | |
| return nil, WrapUnavailable(setErr) | |
| } | |
| return key, nil | |
| default: | |
| return nil, WrapUnavailable(err) | |
| encodedKey, err := getFn() | |
| switch { | |
| case err == nil: | |
| key, decodeErr := base64.StdEncoding.DecodeString(encodedKey) | |
| if decodeErr != nil { | |
| return nil, fmt.Errorf("decode master key: %w", decodeErr) | |
| } | |
| if len(key) != masterKeyBytes { | |
| return nil, fmt.Errorf("invalid master key length: %d", len(key)) | |
| } | |
| return key, nil | |
| case errors.Is(err, keyring.ErrNotFound): | |
| key, genErr := generateFn() | |
| if genErr != nil { | |
| return nil, genErr | |
| } | |
| if setErr := setFn(base64.StdEncoding.EncodeToString(key)); setErr != nil { | |
| return nil, WrapUnavailable(setErr) | |
| } | |
| return key, nil | |
| default: | |
| return nil, WrapUnavailable(err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/keychain_darwin.go` around lines 37 - 58, The current logic
in the getFn success branch incorrectly wraps base64 decode and master-key
length failures with WrapUnavailable, causing corrupted key entries to be
treated as "unavailable" and trigger fallbacks; change the behavior in the
branch where err == nil so that decode errors (from
base64.StdEncoding.DecodeString) and invalid length checks against
masterKeyBytes are returned as direct hard errors (e.g., fmt.Errorf or a
domain-specific error) instead of WrapUnavailable, while preserving
WrapUnavailable only for true keychain access failures (cases where
errors.Is(err, keyring.ErrNotFound) or other getFn errors); keep generation via
generateFn and storage via setFn unchanged.
| // Shared encrypted-file read semantics live in file_encrypted_store.go. | ||
| // New code should reuse that helper layer instead of reimplementing file I/O here. | ||
| return readEncryptedFile(StorageDir(service), account, key) | ||
| } | ||
|
|
||
| func platformSet(service, account, data string) error { | ||
| key, err := getMasterKey(service) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| dir := StorageDir(service) | ||
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return err | ||
| } | ||
| encrypted, err := encryptData(data, key) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| targetPath := filepath.Join(dir, safeFileName(account)) | ||
| tmpPath := filepath.Join(dir, safeFileName(account)+"."+uuid.New().String()+".tmp") | ||
| defer os.Remove(tmpPath) | ||
|
|
||
| if err := os.WriteFile(tmpPath, encrypted, 0600); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Atomic rename to prevent file corruption during multi-process writes | ||
| if err := os.Rename(tmpPath, targetPath); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| // Shared encrypted-file write semantics live in file_encrypted_store.go. | ||
| // New code should reuse that helper layer instead of reimplementing file I/O here. | ||
| return writeEncryptedFile(StorageDir(service), account, data, key) | ||
| } | ||
|
|
||
| func platformRemove(service, account string) error { | ||
| err := os.Remove(filepath.Join(StorageDir(service), safeFileName(account))) | ||
| if err != nil && !os.IsNotExist(err) { | ||
| return err | ||
| } | ||
| return nil | ||
| // Shared encrypted-file cleanup semantics live in file_encrypted_store.go. | ||
| // New code should reuse that helper layer instead of reimplementing file I/O here. | ||
| return removeEncryptedFile(StorageDir(service), account) |
There was a problem hiding this comment.
Preserve the legacy Darwin filename mapping on read/remove.
These calls now resolve primary macOS ciphertext paths through the shared safeFileName implementation, which uses raw-url-base64 filenames. The previous Darwin implementation used a different regexp-based filename mapping, so existing secrets/tokens written before this change will stop being found after upgrade unless you keep a legacy lookup/migration path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/keychain/keychain_darwin.go` around lines 100 - 118, The Darwin
read/remove paths now only use the new safeFileName/raw-url-base64 filenames and
will miss pre-upgrade items; update the platform read function (the one that
currently returns readEncryptedFile(StorageDir(service), account, key)) and
platformRemove to fall back to the legacy Darwin filename mapping: after
computing key via getMasterKey(service) and attempting the normal
readEncryptedFile(StorageDir(service), account, key) (or removeEncryptedFile),
if that returns “not found”, try the legacy regexp-based filename variants used
by the old Darwin implementation (reconstruct the old filename mapping logic and
attempt readEncryptedFile/removeEncryptedFile for those legacy names under
StorageDir(service)); keep using readEncryptedFile, removeEncryptedFile,
safeFileName, getMasterKey and StorageDir so only lookup/migration logic
changes.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/config/init.go (1)
249-259:⚠️ Potential issue | 🟠 MajorThis unchanged-secret branch still bypasses the new cleanup path.
Unlike the plain-secret branches, this one writes
existingback directly and never callscleanupOldConfig(). If a legacy multi-app config is loaded, extra app entries and their stored tokens survive afterconfig initeven though the rest of the command now normalizes through a single save/cleanup flow. Please reusestoreAndSaveOnlyApp, or add an equivalent single-app save + cleanup path here if you need to preserve current users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/init.go` around lines 249 - 259, The unchanged-secret branch updates existing.Apps[0] and calls core.SaveMultiAppConfig, which bypasses the new single-app cleanup path (cleanupOldConfig/storeAndSaveOnlyApp) and leaves legacy extra entries/tokens; modify this branch to reuse the single-app save+cleanup flow by calling the existing helper storeAndSaveOnlyApp (or implement the equivalent: build a single-app config from existing.Apps[0] with updated AppId/Brand/Lang, then call cleanupOldConfig and the single-app save routine) instead of calling core.SaveMultiAppConfig directly after validateSecretReuse.internal/auth/token_store.go (1)
74-92:⚠️ Potential issue | 🟠 MajorReconcile keychain and fallback reads instead of trusting keychain first.
After the fallback write on Lines 109-113, an older keychain entry can still remain because the keychain was unavailable. This read path returns that stale value as soon as keychain access recovers and never reaches the newer fallback copy. Please compare both stores and choose the newer token (for example by
GrantedAt), or otherwise make the fallback authoritative until a later primary write succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/token_store.go` around lines 74 - 92, The current read flow in token_store.go calls tokenKeychain.Get(...) then keychain.GetFallbackWithError(...), but returns the first non-empty keychain result which can be stale; modify the logic in the function that reads tokens (uses tokenKeychain.Get, keychain.GetFallbackWithError, accountKey, appId, userOpenId and unmarshals into StoredUAToken) to read both sources first, unmarshal both into StoredUAToken, and then choose the most recent token by comparing the GrantedAt timestamps (or else treat the fallback as authoritative until a subsequent primary write succeeds); ensure you still call readLegacyManagedToken(appId, userOpenId) when both fail appropriately and only return nil on real errors.
♻️ Duplicate comments (1)
internal/keychain/file_encrypted_store.go (1)
23-25:⚠️ Potential issue | 🟠 MajorUse a lowercase-only filename derivation here.
base64.RawURLEncodingis only collision-free on case-sensitive filesystems. On the default case-insensitive macOS volumes this fallback targets, two different account keys whose encodings differ only by letter case can alias the same.encpath and overwrite each other. Please switch to a lowercase-only deterministic mapping such as hex-encoded SHA-256.Possible fix
import ( "crypto/aes" "crypto/cipher" "crypto/rand" - "encoding/base64" + "crypto/sha256" + "encoding/hex" "fmt" "os" "path/filepath" @@ func safeFileName(account string) string { - return base64.RawURLEncoding.EncodeToString([]byte(account)) + ".enc" + sum := sha256.Sum256([]byte(account)) + return hex.EncodeToString(sum[:]) + ".enc" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/file_encrypted_store.go` around lines 23 - 25, The safeFileName function currently uses base64.RawURLEncoding which can produce mixed-case filenames and collide on case-insensitive filesystems; replace its implementation so it deterministically computes a lowercase-only hex SHA-256 digest of the account string and appends ".enc" (e.g., compute SHA-256(account) and hex-encode the sum in lowercase), updating any imports as needed; keep the function name safeFileName and the returned suffix ".enc".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/config/init.go`:
- Around line 249-259: The unchanged-secret branch updates existing.Apps[0] and
calls core.SaveMultiAppConfig, which bypasses the new single-app cleanup path
(cleanupOldConfig/storeAndSaveOnlyApp) and leaves legacy extra entries/tokens;
modify this branch to reuse the single-app save+cleanup flow by calling the
existing helper storeAndSaveOnlyApp (or implement the equivalent: build a
single-app config from existing.Apps[0] with updated AppId/Brand/Lang, then call
cleanupOldConfig and the single-app save routine) instead of calling
core.SaveMultiAppConfig directly after validateSecretReuse.
In `@internal/auth/token_store.go`:
- Around line 74-92: The current read flow in token_store.go calls
tokenKeychain.Get(...) then keychain.GetFallbackWithError(...), but returns the
first non-empty keychain result which can be stale; modify the logic in the
function that reads tokens (uses tokenKeychain.Get,
keychain.GetFallbackWithError, accountKey, appId, userOpenId and unmarshals into
StoredUAToken) to read both sources first, unmarshal both into StoredUAToken,
and then choose the most recent token by comparing the GrantedAt timestamps (or
else treat the fallback as authoritative until a subsequent primary write
succeeds); ensure you still call readLegacyManagedToken(appId, userOpenId) when
both fail appropriately and only return nil on real errors.
---
Duplicate comments:
In `@internal/keychain/file_encrypted_store.go`:
- Around line 23-25: The safeFileName function currently uses
base64.RawURLEncoding which can produce mixed-case filenames and collide on
case-insensitive filesystems; replace its implementation so it deterministically
computes a lowercase-only hex SHA-256 digest of the account string and appends
".enc" (e.g., compute SHA-256(account) and hex-encode the sum in lowercase),
updating any imports as needed; keep the function name safeFileName and the
returned suffix ".enc".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aa94aee-4710-4c30-8672-1c387a6fd295
📒 Files selected for processing (19)
cmd/auth/login.gocmd/auth/login_test.gocmd/config/init.gocmd/config/init_command_test.gocmd/config/init_storage_test.gocmd/doctor/doctor.gointernal/auth/token_store.gointernal/auth/token_store_test.gointernal/auth/uat_client.gointernal/core/secret.gointernal/core/secret_resolve.gointernal/core/secret_resolve_test.gointernal/keychain/errors.gointernal/keychain/fallback_policy_test.gointernal/keychain/file_encrypted_store.gointernal/keychain/file_encrypted_store_test.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_other.go
✅ Files skipped from review due to trivial changes (9)
- cmd/config/init_command_test.go
- internal/keychain/fallback_policy_test.go
- cmd/doctor/doctor.go
- internal/auth/uat_client.go
- internal/auth/token_store_test.go
- internal/keychain/file_encrypted_store_test.go
- internal/core/secret_resolve_test.go
- internal/keychain/keychain_darwin.go
- cmd/config/init_storage_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/auth/login_test.go
- internal/keychain/keychain_darwin_test.go
- internal/core/secret.go
- internal/keychain/keychain_other.go
- internal/core/secret_resolve.go
| "os" | ||
| "path/filepath" | ||
|
|
There was a problem hiding this comment.
The safeFileName implementation was changed from regex-sanitization (replacing special characters with underscores) to raw URL-encoding. These two schemes produce completely different on-disk names for the same account key, so any credentials written by the old format will never be found by the new code.
Both keychain_darwin.go and keychain_other.go now delegate their platformGet/Set/Remove implementations to the shared helpers in this file, meaning every pre-existing encrypted credential file on macOS under ~/Library/Application Support/<service>/ and on Linux under ~/.local/share/<service>/ will be silently ignored after this upgrade. Users will lose access to their stored app secrets and auth tokens on first run.
The PR correctly adds readLegacyManagedToken to migrate old plaintext token files, but there is no equivalent migration for the old encrypted-file naming convention. A migration path such as falling back to the old sanitized name when the new-format file is absent, or renaming existing files on first access, is needed to avoid silent credential loss on upgrade.
…out_keychain_on_macosx # Conflicts: # cmd/auth/login.go # cmd/doctor/doctor.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/doctor/doctor.go (1)
67-70: Test coverage missing for the new "warn" status.The relevant test file (
doctor_test.go) only covers "pass", "fail", and "skip" statuses. There's no test verifying that "warn" status is treated as non-failing (i.e.,allOKremainstrue). Consider adding a test case that includeswarn()results to confirm the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/doctor/doctor.go` around lines 67 - 70, Add a unit test in doctor_test.go that exercises the new warn status by calling warn(name,msg,hint) and including its result in the checks list used to compute allOK; assert that the warn check produces Status "warn" and that allOK remains true (i.e., warn is non-failing). Reference the warn function and the code path that computes allOK (the function that iterates checkResult values and sets allOK) so the test verifies both the Status string and that warn does not flip allOK to false.
🤖 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/doctor/doctor.go`:
- Around line 67-70: Add a unit test in doctor_test.go that exercises the new
warn status by calling warn(name,msg,hint) and including its result in the
checks list used to compute allOK; assert that the warn check produces Status
"warn" and that allOK remains true (i.e., warn is non-failing). Reference the
warn function and the code path that computes allOK (the function that iterates
checkResult values and sets allOK) so the test verifies both the Status string
and that warn does not flip allOK to false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bda8a15-9050-47b8-9ecf-ea88a62d4182
📒 Files selected for processing (2)
cmd/auth/login.gocmd/doctor/doctor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/auth/login.go
Overview
This PR implements a fallback mechanism for credential storage. It ensures that
lark-cliremains fully functional even when the macOS native keychain is inaccessible due to sandbox restrictions or environment limitations.Closes: #147
Core Changes
1. Encrypted Storage Layer (
internal/keychain)file_encrypted_store.go: Centralizes AES-GCM encryption logic. It manages amaster.keyand stores encrypted secrets in~/.lark-cli/keychain/.ErrUnavailableandShouldUseFallbackto strictly define when the CLI should move from primary to secondary storage.keychain_darwin.goandkeychain_other.goby moving duplicated file I/O logic into the new shared encrypted store.2. Enhanced Configuration Workflow (
cmd/config)config initto use a centralized storage helper (storeAndSaveOnlyApp) that handles the store -> save -> cleanup sequence.validateSecretReuseto ensure users must re-enter secrets if they change an App ID, preventing credential mismatches.3. Architecture & Cleanup
internal/configdir: Created a dedicated package for resolving the CLI's home directory, replacing scatteredos.Getenvandos.UserHomeDircalls.internal/authto support reading/writing tokens to the new fallback store while maintaining backward compatibility with legacy unencrypted token files.Security Considerations
0600permissions (read/write for the owner only).Testing Performed
init_storage_test.go: Verified fallback triggers when the keychain returns "denied."file_encrypted_store_test.go: Confirmed encryption/decryption works as expected and storage artifacts are cleaned up on removal.Checklist
Summary by CodeRabbit
Reliability Improvements
New Features
Bug Fixes