feat: improve OS keychain/DPAPI access error handling for sandbox environments#173
Merged
JackZhao10086 merged 6 commits intomainfrom Apr 1, 2026
Merged
feat: improve OS keychain/DPAPI access error handling for sandbox environments#173JackZhao10086 merged 6 commits intomainfrom
JackZhao10086 merged 6 commits intomainfrom
Conversation
…tforms - Change platformGet to return error instead of empty string - Add proper error wrapping for keychain operations - Make master key creation conditional in getMasterKey - Improve error messages and handling for keychain access - Update dependent code to handle new error returns
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughKeychain APIs were changed to return and normalize errors; platform implementations now distinguish missing, corrupted, and blocked states and allow conditional master-key creation for writes. Call sites (token lookup and config secret resolution) were updated to handle or propagate these errors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant KeychainCore
participant Platform
participant Storage
Caller->>KeychainCore: Get(service, account)
KeychainCore->>Platform: platformGet(service, account) (allowCreate=false for reads)
Platform->>Storage: read encrypted account / registry / file
alt account not found
Storage-->>Platform: not found
Platform-->>KeychainCore: "", nil
else value or error
Storage-->>Platform: value OR io/crypto error OR blocked
Platform-->>KeychainCore: value, error
end
KeychainCore->>KeychainCore: wrapError(op, err) -> possibly ExitError (with hint)
KeychainCore-->>Caller: (value, error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/auth/token_store.go (1)
40-51:⚠️ Potential issue | 🟠 MajorKeychain errors are silently suppressed, defeating the PR's intent.
GetStoredTokenswallows the keychain error and returnsnil, which callers (e.g.,GetValidAccessTokeninuat_client.go:66-99) interpret as "token not found" and trigger re-authentication. This means:
- If the OS keychain is locked or permission is denied, users see a misleading "please re-login" flow instead of the actionable hint about keychain/sandbox permissions.
- The
wrapErrorimprovements in this PR become ineffective for token retrieval paths.Consider changing the signature to
GetStoredToken(appId, userOpenId string) (*StoredUAToken, error)and propagating the keychain error to callers so they can distinguish "not found" from "access denied".Proposed signature change
-// GetStoredToken reads the stored UAT for a given (appId, userOpenId) pair. -func GetStoredToken(appId, userOpenId string) *StoredUAToken { - jsonStr, err := keychain.Get(keychain.LarkCliService, accountKey(appId, userOpenId)) - if err != nil || jsonStr == "" { - return nil - } +// GetStoredToken reads the stored UAT for a given (appId, userOpenId) pair. +// Returns (nil, nil) if no token exists; returns (nil, err) on keychain access failure. +func GetStoredToken(appId, userOpenId string) (*StoredUAToken, error) { + jsonStr, err := keychain.Get(keychain.LarkCliService, accountKey(appId, userOpenId)) + if err != nil { + // Distinguish "not found" from actual access errors + // Platform implementations return specific errors for "not found" + return nil, err + } + if jsonStr == "" { + return nil, nil + } var token StoredUAToken if err := json.Unmarshal([]byte(jsonStr), &token); err != nil { - return nil + return nil, nil // Treat corrupted data as missing } - return &token + return &token, nil }🤖 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 40 - 51, GetStoredToken currently swallows keychain.Get errors and returns nil, which prevents callers like GetValidAccessToken from distinguishing "not found" vs keychain/access errors; change the signature of GetStoredToken(appId, userOpenId string) to return (*StoredUAToken, error), have it return (nil, nil) only when jsonStr=="" (not when err != nil), propagate and wrap keychain.Get errors (use wrapError) and return (nil, err) on JSON unmarshal failures or keychain failures, then update callers such as GetValidAccessToken in uat_client.go to handle the error path and show actionable messages for keychain access issues.
🧹 Nitpick comments (4)
internal/keychain/keychain_darwin.go (2)
73-77: Same issue: "keychain is corrupted" is misleading for missing master key.When
keyring.ErrNotFoundoccurs andallowCreateis false (read operation), returning "keychain is corrupted" suggests data corruption rather than "entry not initialized". This is the same issue as inkeychain_other.go.Suggested fix
if !allowCreate { - // Creation not allowed (e.g., during Get operation), return error - resCh <- result{key: nil, err: errors.New("keychain is corrupted")} + // Creation not allowed (e.g., during Get operation), return not-found error + resCh <- result{key: nil, err: errors.New("keychain entry not found")} return }🤖 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 73 - 77, The error returned when allowCreate is false currently uses a misleading "keychain is corrupted" message; update the branch in the function that sends to resCh (when !allowCreate) to return a clearer error indicating the master key/entry is missing/not initialized (e.g., "master key not initialized" or wrap keyring.ErrNotFound) instead of "keychain is corrupted", matching the behavior used in keychain_other.go; adjust the error value sent via resCh's result{key: nil, err: ...} accordingly.
53-54: Silentrecover()may mask goroutine panics.The
defer func() { recover() }()swallows any panic without logging, which could hide bugs in the keychain access logic. Consider at minimum logging the panic or removing the recover if panics here indicate programming errors that should propagate.Suggested improvement
go func() { - defer func() { recover() }() + defer func() { + if r := recover(); r != nil { + resCh <- result{key: nil, err: fmt.Errorf("keychain panic: %v", r)} + } + }()🤖 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 53 - 54, The anonymous goroutine in keychain_darwin.go uses defer func() { recover() }() which silently swallows panics; update the deferred recover to capture the recovered value and surface it (e.g., log the panic with context via log.Printf or processLogger.Errorf) or remove the recover entirely if a panic should crash to surface programming errors; specifically modify the goroutine's deferred function in the anonymous func() to check r := recover(); if r != nil { log the error and stack (runtime/debug.Stack()) with a clear message including "keychain_darwin goroutine" or the surrounding function name } so panics are not silently ignored.internal/keychain/keychain.go (1)
41-46: Stale comment:Getnow returns an error, not empty string.The comment on line 42 says "Returns empty string if the entry does not exist" but the function now returns
(string, error). The error path handles the "not found" case.Suggested fix
// Get retrieves a value from the keychain. -// Returns empty string if the entry does not exist. +// Returns an error if the entry does not exist or cannot be accessed. func Get(service, account string) (string, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain.go` around lines 41 - 46, Update the stale doc comment for the Get function in keychain.go: change "Returns empty string if the entry does not exist" to reflect that Get now returns (string, error) and that missing entries are reported via the returned error (wrapped by wrapError in platformGet path); ensure the top comment describes returned values and the error semantics for not-found and other failures.internal/keychain/keychain_other.go (1)
53-55: Error message "keychain is corrupted" is misleading for missing master key.When a user runs a
Getoperation before anySethas occurred (e.g., fresh install, querying non-existent entry), the master key won't exist. Returning "keychain is corrupted" suggests data corruption when the actual state is "not initialized" or "entry not found".Consider a more accurate message:
Suggested fix
if !allowCreate { - return nil, errors.New("keychain is corrupted") + return nil, errors.New("keychain not initialized or entry not found") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_other.go` around lines 53 - 55, The error message returned when allowCreate is false is misleading; update the return in the branch that currently does `return nil, errors.New("keychain is corrupted")` to a clearer message indicating missing master key or uninitialized keychain (for example `errors.New("keychain not initialized: master key missing")` or `errors.New("entry not found or keychain not initialized")`). Locate the branch that checks `if !allowCreate` in the Get (or related) method in keychain_other.go and replace the error text accordingly so callers can distinguish "not initialized / missing entry" from actual corruption.
🤖 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 `@internal/auth/token_store.go`:
- Around line 40-51: GetStoredToken currently swallows keychain.Get errors and
returns nil, which prevents callers like GetValidAccessToken from distinguishing
"not found" vs keychain/access errors; change the signature of
GetStoredToken(appId, userOpenId string) to return (*StoredUAToken, error), have
it return (nil, nil) only when jsonStr=="" (not when err != nil), propagate and
wrap keychain.Get errors (use wrapError) and return (nil, err) on JSON unmarshal
failures or keychain failures, then update callers such as GetValidAccessToken
in uat_client.go to handle the error path and show actionable messages for
keychain access issues.
---
Nitpick comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 73-77: The error returned when allowCreate is false currently uses
a misleading "keychain is corrupted" message; update the branch in the function
that sends to resCh (when !allowCreate) to return a clearer error indicating the
master key/entry is missing/not initialized (e.g., "master key not initialized"
or wrap keyring.ErrNotFound) instead of "keychain is corrupted", matching the
behavior used in keychain_other.go; adjust the error value sent via resCh's
result{key: nil, err: ...} accordingly.
- Around line 53-54: The anonymous goroutine in keychain_darwin.go uses defer
func() { recover() }() which silently swallows panics; update the deferred
recover to capture the recovered value and surface it (e.g., log the panic with
context via log.Printf or processLogger.Errorf) or remove the recover entirely
if a panic should crash to surface programming errors; specifically modify the
goroutine's deferred function in the anonymous func() to check r := recover();
if r != nil { log the error and stack (runtime/debug.Stack()) with a clear
message including "keychain_darwin goroutine" or the surrounding function name }
so panics are not silently ignored.
In `@internal/keychain/keychain_other.go`:
- Around line 53-55: The error message returned when allowCreate is false is
misleading; update the return in the branch that currently does `return nil,
errors.New("keychain is corrupted")` to a clearer message indicating missing
master key or uninitialized keychain (for example `errors.New("keychain not
initialized: master key missing")` or `errors.New("entry not found or keychain
not initialized")`). Locate the branch that checks `if !allowCreate` in the Get
(or related) method in keychain_other.go and replace the error text accordingly
so callers can distinguish "not initialized / missing entry" from actual
corruption.
In `@internal/keychain/keychain.go`:
- Around line 41-46: Update the stale doc comment for the Get function in
keychain.go: change "Returns empty string if the entry does not exist" to
reflect that Get now returns (string, error) and that missing entries are
reported via the returned error (wrapped by wrapError in platformGet path);
ensure the top comment describes returned values and the error semantics for
not-found and other failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1010b21-42be-4df9-be04-703dd96f6153
📒 Files selected for processing (7)
internal/auth/token_store.gointernal/core/config.gointernal/keychain/default.gointernal/keychain/keychain.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.gointernal/keychain/keychain_windows.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@0b8e1f9883312254503e81cd09ee5af8984ff678🧩 Skill updatenpx skills add larksuite/cli#feat/adapt_to_sandbox_mode -y -g |
Greptile SummaryThis PR improves cross-platform keychain error handling so that access failures in sandboxed or CI environments surface as actionable Key changes:
Confidence Score: 4/5Safe to merge on macOS/Linux with noted caveats; Windows DPAPI errors and missing-secret scenarios on all platforms remain silent Several issues raised in prior review threads remain unresolved: Windows
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["keychain.Get(service, account)"] --> B["platformGet(service, account)"]
subgraph Darwin_Linux["Darwin / Linux"]
B --> C["os.ReadFile(enc file)"]
C -->|ErrNotExist| D["return empty, nil"]
C -->|other error| E["return err"]
C -->|success| F["getMasterKey(false)"]
F -->|ErrNotFound + no allowCreate| G["return errNotInitialized"]
F -->|access blocked or timeout| H["return keychain access blocked"]
F -->|corrupted key| I["return keychain is corrupted"]
F -->|success| J["decryptData(data, key)"]
J -->|fail| K["return err — wrong hint applied"]
J -->|success| L["return plaintext"]
end
subgraph Windows_Backend["Windows"]
B --> W["registryGet(service, account)"]
W -->|ok=true| X["return value, nil"]
W -->|ok=false| Y["return empty, nil — silent"]
end
E --> Z["wrapError('Get', err)"]
G --> Z
H --> Z
I --> Z
K --> Z
Z --> AA["ExitError with hint"]
AA -->|errNotInitialized| AB["hint: run lark-cli config init"]
AA -->|all other errors| AC["hint: check locked/accessible"]
Reviews (6): Last reviewed commit: "fix(keychain): handle I/O errors when re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Add detailed doc comments for all platform-specific keychain functions to clarify their purpose and behavior. Also enhance the error hint message to include a suggestion for reconfiguring the CLI when keychain access fails.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 154-162: In platformGet, don't call getMasterKey before checking
for the ciphertext file: first attempt to read the file at
filepath.Join(StorageDir(service), safeFileName(account)), and if
os.IsNotExist(err) return "", nil; only when the ciphertext file exists should
you call getMasterKey(service, false) and proceed with decryption; update error
handling so missing files produce the documented ("", nil) and corrupted key
errors still surface when getMasterKey or decryption fails.
- Around line 75-95: There is a race in getMasterKey when allowCreate==true:
concurrent callers can both observe ErrNotFound, generate different master keys,
and both call keyring.Set (upserting) so the last write wins and breaks existing
encrypted data; fix by adding a service-scoped file lock (e.g., based on the
service string) around the creation path in getMasterKey (or invoked from
platformSet) so only one goroutine can perform generation+set at a time, and
after acquiring the lock re-check the keyring for an existing "master.key"
(handle ErrNotFound vs found) before generating a new random key, then only call
keyring.Set if still missing; ensure the lock is released on all code paths and
propagate any errors from rand.Read or keyring.Set as before.
🪄 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: fb859568-85ca-49ef-aeb2-126dc6b589b7
📒 Files selected for processing (4)
internal/keychain/keychain.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.gointernal/keychain/keychain_windows.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/keychain/keychain_windows.go
- internal/keychain/keychain.go
- internal/keychain/keychain_other.go
…r handling Check for file existence before attempting to read and get master key
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/keychain/keychain_darwin.go (2)
66-72: Non-sentinel errors prevent reliable error classification by callers.The errors "keychain is corrupted" and "keychain access blocked" are created with
errors.New(), making them unexported string-based errors. Callers (likeResolveSecretInputininternal/core/secret_resolve.go) cannot useerrors.Is()orerrors.As()to distinguish between recoverable and unrecoverable errors.Consider defining exported sentinel errors for programmatic error handling:
Example sentinel error definitions
// In keychain package var ( ErrKeychainCorrupted = errors.New("keychain is corrupted") ErrKeychainBlocked = errors.New("keychain access blocked") )Then callers can check:
if errors.Is(err, keychain.ErrKeychainBlocked) { // Handle sandbox/CI environment }🤖 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 66 - 72, Define exported sentinel errors in the keychain package (e.g., ErrKeychainCorrupted and ErrKeychainBlocked) and replace the inline errors.New(...) creations in keychain_darwin.go with those exported variables where the code currently sends result{..., err: errors.New("keychain is corrupted")} and result{..., err: errors.New("keychain access blocked")}; update callers (such as ResolveSecretInput in internal/core/secret_resolve.go) to use errors.Is(err, keychain.ErrKeychainBlocked) / errors.Is(err, keychain.ErrKeychainCorrupted) for programmatic classification.
66-79: Misleading error message: "keychain is corrupted" for missing key.Line 78 returns "keychain is corrupted" when
allowCreate=falseand the key simply doesn't exist (ErrNotFound). This conflates two distinct scenarios:
- Key exists but is invalid/corrupted (lines 66-68)
- Key was never created (line 78)
For a fresh install where no
Sethas occurred, aGetoperation will return "keychain is corrupted" which is misleading to users. Consider using distinct error messages like "keychain not initialized" for the ErrNotFound case.Suggested differentiation
// If ErrNotFound, check if we are allowed to create a new key if !allowCreate { // Creation not allowed (e.g., during Get operation), return error - resCh <- result{key: nil, err: errors.New("keychain is corrupted")} + resCh <- result{key: nil, err: errors.New("keychain not initialized")} return }🤖 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 66 - 79, The error message conflates a missing key (ErrNotFound) with a corrupted key; update the branch that handles ErrNotFound when allowCreate is false to return a distinct error (e.g., errors.New("keychain not initialized") or similar) instead of "keychain is corrupted". Locate the block that checks errors.Is(err, keyring.ErrNotFound) and the subsequent if !allowCreate branch that sends on resCh with result{key: nil, err: ...} and change that error text; keep the existing handling for the corrupted-key branch that sends "keychain is corrupted". Ensure callers still receive the new, clearer error when no key exists and creation is not allowed.internal/keychain/keychain_other.go (2)
26-35: Warning message printed even when home directory is valid.Lines 28-32 print a warning when
err != nil || home == "", but line 33 continues to usehomeeven when it might be empty. Ifhome == ""(buterr == nil), the warning mentions%vwith anilerror, producing output like:warning: unable to determine home directory: <nil>Consider only printing the warning when there's an actual error:
Suggested fix
func StorageDir(service string) string { home, err := os.UserHomeDir() - if err != nil || home == "" { - // If home is missing, fallback to relative path and print warning. - // This matches the behavior in internal/core/config.go. - fmt.Fprintf(os.Stderr, "warning: unable to determine home directory: %v\n", err) + if err != nil { + fmt.Fprintf(os.Stderr, "warning: unable to determine home directory: %v\n", err) + } + if home == "" { + // Fallback handled below - xdgData will be ".local/share" } xdgData := filepath.Join(home, ".local", "share") return filepath.Join(xdgData, service) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_other.go` around lines 26 - 35, In StorageDir, the current check prints a warning when home=="" even if err==nil and then still uses home; change the logic so you only call fmt.Fprintf when err != nil (to avoid "nil" in the log) and separately handle an empty home by falling back to a relative path (e.g. set home = ".") before computing xdgData and returning filepath.Join(xdgData, service); update the checks around the variables home and err accordingly so StorageDir never uses an empty home and never prints a nil error.
50-57: Same misleading error issue: "keychain is corrupted" for missing key.Similar to
keychain_darwin.go, when the master key file doesn't exist andallowCreate=false, returning "keychain is corrupted" is misleading. This scenario occurs on a fresh install whenplatformGetis called before anyplatformSet.Lines 50-53 check if the key exists and is valid. If the file doesn't exist or has wrong length, and
allowCreate=false, line 56 returns "keychain is corrupted" even though nothing is actually corrupted—the keychain was simply never initialized.Suggested fix
if !allowCreate { - return nil, errors.New("keychain is corrupted") + return nil, errors.New("keychain not initialized") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_other.go` around lines 50 - 57, The error message "keychain is corrupted" in keychain_other.go is misleading when the master key file is simply missing; update the logic in the code handling key loading (the block reading keyPath and the callers platformGet/platformSet) to distinguish file-not-found from a bad-length file: if os.ReadFile returns an error and os.IsNotExist(err) (or equivalent) then, when allowCreate==false, return a clear "keychain not initialized" (or "master key not found") error; if the file exists but len(key) != masterKeyBytes then return "keychain is corrupted" as before. Ensure this change touches the read/validate flow around keyPath and the code paths used by platformGet/platformSet to preserve behavior when allowCreate is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 66-72: Define exported sentinel errors in the keychain package
(e.g., ErrKeychainCorrupted and ErrKeychainBlocked) and replace the inline
errors.New(...) creations in keychain_darwin.go with those exported variables
where the code currently sends result{..., err: errors.New("keychain is
corrupted")} and result{..., err: errors.New("keychain access blocked")}; update
callers (such as ResolveSecretInput in internal/core/secret_resolve.go) to use
errors.Is(err, keychain.ErrKeychainBlocked) / errors.Is(err,
keychain.ErrKeychainCorrupted) for programmatic classification.
- Around line 66-79: The error message conflates a missing key (ErrNotFound)
with a corrupted key; update the branch that handles ErrNotFound when
allowCreate is false to return a distinct error (e.g., errors.New("keychain not
initialized") or similar) instead of "keychain is corrupted". Locate the block
that checks errors.Is(err, keyring.ErrNotFound) and the subsequent if
!allowCreate branch that sends on resCh with result{key: nil, err: ...} and
change that error text; keep the existing handling for the corrupted-key branch
that sends "keychain is corrupted". Ensure callers still receive the new,
clearer error when no key exists and creation is not allowed.
In `@internal/keychain/keychain_other.go`:
- Around line 26-35: In StorageDir, the current check prints a warning when
home=="" even if err==nil and then still uses home; change the logic so you only
call fmt.Fprintf when err != nil (to avoid "nil" in the log) and separately
handle an empty home by falling back to a relative path (e.g. set home = ".")
before computing xdgData and returning filepath.Join(xdgData, service); update
the checks around the variables home and err accordingly so StorageDir never
uses an empty home and never prints a nil error.
- Around line 50-57: The error message "keychain is corrupted" in
keychain_other.go is misleading when the master key file is simply missing;
update the logic in the code handling key loading (the block reading keyPath and
the callers platformGet/platformSet) to distinguish file-not-found from a
bad-length file: if os.ReadFile returns an error and os.IsNotExist(err) (or
equivalent) then, when allowCreate==false, return a clear "keychain not
initialized" (or "master key not found") error; if the file exists but len(key)
!= masterKeyBytes then return "keychain is corrupted" as before. Ensure this
change touches the read/validate flow around keyPath and the code paths used by
platformGet/platformSet to preserve behavior when allowCreate is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 025a78a9-0aea-4ad3-89c8-e9b480345536
📒 Files selected for processing (2)
internal/keychain/keychain_darwin.gointernal/keychain/keychain_other.go
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 (1)
internal/keychain/keychain_other.go (1)
50-57:⚠️ Potential issue | 🟡 MinorInconsistent handling of corrupted master key vs Darwin.
When the master key file exists but has invalid length:
- Darwin: Explicitly returns
errors.New("keychain is corrupted")(line 66-68 in darwin file)- Linux: Falls through to return
errNotInitializedwhenallowCreate=falseThis means Linux users with a corrupted key see "keychain not initialized" instead of "keychain is corrupted", which is less accurate.
Suggested fix for consistency
func getMasterKey(service string, allowCreate bool) ([]byte, error) { dir := StorageDir(service) keyPath := filepath.Join(dir, "master.key") key, err := os.ReadFile(keyPath) if err == nil && len(key) == masterKeyBytes { return key, nil } + if err == nil && len(key) != masterKeyBytes { + // Key file exists but is corrupted + return nil, errors.New("keychain is corrupted") + } if !allowCreate { return nil, errNotInitialized }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_other.go` around lines 50 - 57, When a master key file is present but has the wrong length (os.ReadFile succeeded but len(key) != masterKeyBytes) the code currently falls through and returns errNotInitialized for Linux; change this to return a consistent "keychain is corrupted" error like Darwin does: either return errors.New("keychain is corrupted") in that branch (when allowCreate is false) or better, introduce a shared sentinel (e.g., ErrKeychainCorrupted) and return it instead of errNotInitialized so the corruption case (checked where keyPath, masterKeyBytes and allowCreate are evaluated) is reported consistently across platforms.
🤖 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_windows.go`:
- Around line 110-117: platformGet currently swallows all errors by treating
registryGet failures as "not found"; change registryGet to return (string,
error) and update platformGet to propagate real errors instead of returning ("",
nil). Specifically, modify registryGet to return an error for
DPAPI/registry/base64/decryption failures and return ("", nil) only when the key
truly does not exist, then update platformGet (and any callers) to check the
error from registryGet and return ("", err) on failure so access/permission
errors surface (refer to function names platformGet and registryGet for locating
the changes).
---
Outside diff comments:
In `@internal/keychain/keychain_other.go`:
- Around line 50-57: When a master key file is present but has the wrong length
(os.ReadFile succeeded but len(key) != masterKeyBytes) the code currently falls
through and returns errNotInitialized for Linux; change this to return a
consistent "keychain is corrupted" error like Darwin does: either return
errors.New("keychain is corrupted") in that branch (when allowCreate is false)
or better, introduce a shared sentinel (e.g., ErrKeychainCorrupted) and return
it instead of errNotInitialized so the corruption case (checked where keyPath,
masterKeyBytes and allowCreate are evaluated) is reported consistently across
platforms.
🪄 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: 69b976a6-2354-4df9-b9a3-57aa450acd9a
📒 Files selected for processing (4)
internal/keychain/keychain.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.gointernal/keychain/keychain_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/keychain/keychain.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/keychain/keychain_other.go (1)
189-196: Consider usingerrors.Isfor consistency.
platformGetuseserrors.Is(err, os.ErrNotExist)(line 142), while this function usesos.IsNotExist(err). Both work, buterrors.Isis the modern Go idiom and better handles wrapped errors.♻️ Suggested change for consistency
func platformRemove(service, account string) error { err := os.Remove(filepath.Join(StorageDir(service), safeFileName(account))) - if err != nil && !os.IsNotExist(err) { + if err != nil && !errors.Is(err, os.ErrNotExist) { return err } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_other.go` around lines 189 - 196, The platformRemove function uses os.IsNotExist(err) while platformGet uses errors.Is(err, os.ErrNotExist); update platformRemove (function platformRemove) to check errors.Is(err, os.ErrNotExist) instead of os.IsNotExist to match the modern Go idiom and handle wrapped errors, and ensure the package imports the errors package if not already imported.
🤖 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_other.go`:
- Around line 50-61: The code fails to propagate non-ErrNotExist read errors
from os.ReadFile(keyPath) and wrongly treats them as "not initialized"; update
the error handling around the os.ReadFile call in the function that uses
keyPath, masterKeyBytes and allowCreate so that if err != nil you first check
errors.Is(err, os.ErrNotExist) and only convert that case to errNotInitialized
(or proceed to key creation when allowCreate is true); for any other err
(permission/I/O/etc.) return that error immediately instead of falling through,
keeping the existing logic for the happy-path when len(key) == masterKeyBytes
and the corrupted-key branch.
---
Nitpick comments:
In `@internal/keychain/keychain_other.go`:
- Around line 189-196: The platformRemove function uses os.IsNotExist(err) while
platformGet uses errors.Is(err, os.ErrNotExist); update platformRemove (function
platformRemove) to check errors.Is(err, os.ErrNotExist) instead of os.IsNotExist
to match the modern Go idiom and handle wrapped errors, and ensure the package
imports the errors package if not already imported.
🪄 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: e3d57d8a-8164-421c-a4b4-07a46d6f1770
📒 Files selected for processing (1)
internal/keychain/keychain_other.go
liangshuo-1
approved these changes
Apr 1, 2026
tuxedomm
pushed a commit
that referenced
this pull request
Apr 3, 2026
…ironments (#173) * refactor(keychain): improve error handling and consistency across platforms - Change platformGet to return error instead of empty string - Add proper error wrapping for keychain operations - Make master key creation conditional in getMasterKey - Improve error messages and handling for keychain access - Update dependent code to handle new error returns * docs(keychain): improve function documentation and error message Add detailed doc comments for all platform-specific keychain functions to clarify their purpose and behavior. Also enhance the error hint message to include a suggestion for reconfiguring the CLI when keychain access fails. * refactor(keychain): reorder operations in platformGet for better error handling Check for file existence before attempting to read and get master key * fix(keychain): improve error handling and consistency across platforms. * fix(keychain): handle corrupted master key case * fix(keychain): handle I/O errors when reading master key
This was referenced Apr 7, 2026
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
This PR improves the cross-platform keychain error handling mechanism to better support restricted environments like Sandboxes or CI. It ensures keychain access errors are properly propagated and provides actionable troubleshooting hints when permissions are denied or blocked.
Changes
keychain.GetandplatformGetAPI signatures to return(string, error)for proper error propagation.getMasterKeywith anallowCreateflag to prevent regenerating master keys during read-only operations.wrapErrorto wrap underlying keychain failures into user-friendlyExitErrors with sandbox/CI permission hints.config.goandtoken_store.goto handle and correctly render the new keychain errors.Test Plan
lark auth login) work normally across different platforms (macOS/Windows/Linux).Related Issues
Summary by CodeRabbit