fix: share key retriever across all browsers to avoid repeated prompts#560
fix: share key retriever across all browsers to avoid repeated prompts#560
Conversation
…rd prompts On macOS, each browser (Chrome, Edge, Chromium) created its own KeyRetriever instance, causing the login password prompt to appear once per browser instead of once total. - Create a single shared retriever in pickFromConfigs() and pass it to all Chromium browsers - Fix GcoredumpRetriever to cache all keychain records instead of a single browser's derived key - Fix SecurityCmdRetriever to cache results per storage name instead of only the first browser's result - Simplify terminal prompt to "Enter macOS login password:" since it unlocks the entire keychain
Helps users understand why the tool falls back to the security command dialog after entering a wrong password.
Show a concise user-friendly message at WARN level and move the detailed error chain to DEBUG level for verbose mode.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #560 +/- ##
==========================================
+ Coverage 70.77% 72.41% +1.63%
==========================================
Files 47 47
Lines 1718 1722 +4
==========================================
+ Hits 1216 1247 +31
+ Misses 385 355 -30
- Partials 117 120 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors Chromium key retrieval on macOS so a single KeyRetriever instance is shared across all discovered Chromium-based browsers, reducing repeated password prompts, redundant gcore dumps, and repeated security CLI authorization dialogs.
Changes:
- Lift
KeyRetrievercreation up to browser selection (pickFromConfigs) and pass it down into Chromium browser discovery. - Update macOS retrievers to cache decrypted keychain data across calls (including per-storage caching for
securityCLI lookups). - Change the
gcore-based keychain decrypt helper to return all generic-password records (not a single storage-specific secret).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
browser/browser.go |
Creates one shared KeyRetriever and passes it into Chromium browser creation. |
browser/chromium/chromium.go |
Updates NewBrowsers to accept an injected retriever shared across browsers. |
browser/chromium/chromium_test.go |
Adjusts tests for the new NewBrowsers signature. |
crypto/keyretriever/keyretriever_darwin.go |
Updates macOS retrievers to cache records and adds per-storage caching for security CLI retrieval. |
crypto/keyretriever/gcoredump_darwin.go |
Renames/changes DecryptKeychain to return all generic-password records from the keychain. |
Comments suppressed due to low confidence (1)
browser/chromium/chromium.go:47
NewBrowsersnow requires a non-nilretriever, but the function doesn’t validate it and latergetMasterKey()unconditionally callsb.retriever.RetrieveKey(...), which will panic if a caller passesnil(the tests currently do). Consider preserving the previous behavior by defaultingretrievertokeyretriever.DefaultRetriever(cfg.KeychainPassword)when nil, or return a clear error if nil is not allowed.
// NewBrowsers discovers Chromium profiles under cfg.UserDataDir and returns
// one Browser per profile. The retriever is shared across all browsers to
// avoid repeated keychain password prompts on macOS.
func NewBrowsers(cfg types.BrowserConfig, retriever keyretriever.KeyRetriever) ([]*Browser, error) {
sources := sourcesForKind(cfg.Kind)
extractors := extractorsForKind(cfg.Kind)
profileDirs := discoverProfiles(cfg.UserDataDir, sources)
if len(profileDirs) == 0 {
return nil, nil
}
var browsers []*Browser
for _, profileDir := range profileDirs {
sourcePaths := resolveSourcePaths(sources, profileDir)
if len(sourcePaths) == 0 {
continue
}
browsers = append(browsers, &Browser{
cfg: cfg,
profileDir: profileDir,
retriever: retriever,
sources: sources,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| r.records, r.err = loadKeychainRecords(string(pwd)) | ||
| if r.err != nil { | ||
| log.Warnf("incorrect password, falling back to security command") |
There was a problem hiding this comment.
TerminalPasswordRetriever logs "incorrect password" on any loadKeychainRecords failure. That error can also be caused by other conditions (e.g., keychain open/unlock failures unrelated to password), making the warning misleading. Consider changing the message to something neutral (e.g., "failed to unlock keychain; falling back to security command") or only using the "incorrect password" wording when you can positively identify that specific failure.
| log.Warnf("incorrect password, falling back to security command") | |
| log.Warnf("failed to unlock keychain; falling back to security command") |
| func (r *SecurityCmdRetriever) RetrieveKey(storage, _ string) ([]byte, error) { | ||
| r.once.Do(func() { | ||
| r.key, r.err = r.retrieveKeyOnce(storage) | ||
| }) | ||
| return r.key, r.err | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
|
|
||
| if r.cache == nil { | ||
| r.cache = make(map[string]securityResult) | ||
| } | ||
| if res, ok := r.cache[storage]; ok { | ||
| return res.key, res.err | ||
| } | ||
|
|
||
| key, err := r.retrieveKeyOnce(storage) | ||
| r.cache[storage] = securityResult{key: key, err: err} | ||
| return key, err |
There was a problem hiding this comment.
SecurityCmdRetriever.RetrieveKey holds mu while running retrieveKeyOnce, which executes an external security command and can block up to securityCmdTimeout. This serializes all concurrent key retrievals (even for different storages) and can stall extraction work. Consider narrowing the critical section (e.g., check cache under lock, run the command without holding the lock, then re-lock to store the result) or using a per-storage singleflight/once mechanism to dedupe in-flight calls.
| // one Browser per profile. The retriever is shared across all browsers to | ||
| // avoid repeated keychain password prompts on macOS. | ||
| func NewBrowsers(cfg types.BrowserConfig, retriever keyretriever.KeyRetriever) ([]*Browser, error) { |
There was a problem hiding this comment.
Changing the exported chromium.NewBrowsers signature is a breaking API change for any external consumers of this module. If this package is intended to be used as a library, consider keeping the old NewBrowsers(cfg) as a wrapper that calls the new form with a default retriever, or introduce a new name (e.g., NewBrowsersWithRetriever) to avoid breaking callers.
| // one Browser per profile. The retriever is shared across all browsers to | |
| // avoid repeated keychain password prompts on macOS. | |
| func NewBrowsers(cfg types.BrowserConfig, retriever keyretriever.KeyRetriever) ([]*Browser, error) { | |
| // one Browser per profile. | |
| // | |
| // This wrapper preserves the original exported API for external callers. | |
| // Callers that need to provide a shared key retriever should use | |
| // NewBrowsersWithRetriever. | |
| func NewBrowsers(cfg types.BrowserConfig) ([]*Browser, error) { | |
| return NewBrowsersWithRetriever(cfg, nil) | |
| } | |
| // NewBrowsersWithRetriever discovers Chromium profiles under cfg.UserDataDir | |
| // and returns one Browser per profile. The retriever is shared across all | |
| // browsers to avoid repeated keychain password prompts on macOS. | |
| func NewBrowsersWithRetriever(cfg types.BrowserConfig, retriever keyretriever.KeyRetriever) ([]*Browser, error) { |
| if len(records) > 0 { | ||
| return records, nil | ||
| } |
There was a problem hiding this comment.
DecryptKeychainRecords treats len(records) == 0 as a failed unlock and keeps trying candidates, eventually returning "none unlocked keychain". It’s possible to successfully unlock the keychain but have zero generic-password items (or none relevant to Chromium), in which case the final error message becomes misleading. Consider returning success once kc.Unlock(...) succeeds (even if records is empty), and let the caller handle “storage not found” separately.
| if len(records) > 0 { | |
| return records, nil | |
| } | |
| return records, nil |
| type GcoredumpRetriever struct { | ||
| once sync.Once | ||
| key []byte | ||
| err error | ||
| once sync.Once | ||
| records []keychainbreaker.GenericPassword | ||
| err error |
There was a problem hiding this comment.
Caching []keychainbreaker.GenericPassword means the process may hold many generic-password items from the user’s login keychain in memory (not just Chromium’s storage secrets). That increases the blast radius of any memory disclosure and may be unnecessary for the goal of avoiding repeated prompts. Consider caching only derived per-storage keys (or a minimal filtered subset of records for known Chromium storage accounts) instead of retaining all keychain Password bytes.
Move KeyRetriever injection from NewBrowsers constructor parameter to post-construction SetRetriever method. This separates profile discovery (filesystem-only) from key retrieval configuration (platform-specific credentials), eliminating three design smells: - Tests no longer pass nil for a dependency they don't use - Firefox dispatch path no longer receives a Chromium-specific parameter - BrowserConfig no longer carries runtime state (KeychainPassword removed) The shared retriever is still created once in pickFromConfigs and injected into all Chromium browsers via the retrieverSetter interface, preserving the fix for repeated macOS keychain password prompts. Add tests for getMasterKey, Extract, newBrowsers dispatcher, and SetRetriever interface compliance.
- TerminalPasswordRetriever: return explicit error when stdin is not a
TTY instead of (nil, nil), so ChainRetriever records the skip reason
- TerminalPasswordRetriever: remove hardcoded fallback knowledge from
log message ("falling back to security command" → generic message)
- SecurityCmdRetriever: initialize cache map at construction time in
DefaultRetriever instead of lazy init inside RetrieveKey
- DecryptKeychainRecords: wrap scanMasterKeyCandidates error with
context for consistent error reporting
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -34,11 +35,6 @@ func NewBrowsers(cfg types.BrowserConfig) ([]*Browser, error) { | |||
| return nil, nil | |||
| } | |||
|
|
|||
| // Create the key retriever once and share it across all profiles. | |||
| // This avoids repeated keychain password prompts on macOS, where each | |||
| // profile would otherwise trigger a separate `security` command dialog. | |||
| retriever := keyretriever.DefaultRetriever(cfg.KeychainPassword) | |||
|
|
|||
| var browsers []*Browser | |||
| for _, profileDir := range profileDirs { | |||
| sourcePaths := resolveSourcePaths(sources, profileDir) | |||
| @@ -48,7 +44,6 @@ func NewBrowsers(cfg types.BrowserConfig) ([]*Browser, error) { | |||
| browsers = append(browsers, &Browser{ | |||
| cfg: cfg, | |||
| profileDir: profileDir, | |||
| retriever: retriever, | |||
| sources: sources, | |||
| extractors: extractors, | |||
| sourcePaths: sourcePaths, | |||
| @@ -57,6 +52,13 @@ func NewBrowsers(cfg types.BrowserConfig) ([]*Browser, error) { | |||
| return browsers, nil | |||
There was a problem hiding this comment.
chromium.NewBrowsers now accepts a retriever parameter, but the value is stored and later dereferenced in getMasterKey without a nil check. Passing nil will lead to a panic at runtime when Extract calls b.retriever.RetrieveKey(...). Consider returning an error from NewBrowsers when retriever == nil, or defaulting to keyretriever.DefaultRetriever("") so callers can’t accidentally construct a crashing Browser.
| r.records, r.err = loadKeychainRecords(string(pwd)) | ||
| if r.err != nil { | ||
| log.Warnf("keychain unlock failed with provided password") | ||
| log.Debugf("keychain unlock detail: %v", r.err) | ||
| } |
There was a problem hiding this comment.
The warning log on keychain unlock failure says "incorrect password", but loadKeychainRecords can fail for reasons other than a wrong password (e.g., keychain open/unlock errors, corrupted DB). This message can mislead users troubleshooting failures. Prefer a more general warning (e.g., "failed to unlock keychain with provided password") and rely on the debug log to include the underlying error detail.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TerminalPasswordRetriever prompts for the keychain password interactively | ||
| // via the terminal using golang.org/x/term (with echo disabled). | ||
| // Automatically skipped when stdin is not a TTY. | ||
| type TerminalPasswordRetriever struct { | ||
| once sync.Once | ||
| records []keychainbreaker.GenericPassword | ||
| err error | ||
| } | ||
|
|
||
| func (r *TerminalPasswordRetriever) RetrieveKey(storage, _ string) ([]byte, error) { | ||
| if !term.IsTerminal(int(os.Stdin.Fd())) { | ||
| return nil, nil | ||
| return nil, fmt.Errorf("terminal: stdin is not a TTY") | ||
| } |
There was a problem hiding this comment.
TerminalPasswordRetriever now returns an error when stdin is not a TTY, but the type comment still says it is "Automatically skipped". Update the comment (or adjust behavior) so documentation matches the new semantics (error vs silent skip).
| func (r *SecurityCmdRetriever) RetrieveKey(storage, _ string) ([]byte, error) { | ||
| r.once.Do(func() { | ||
| r.key, r.err = r.retrieveKeyOnce(storage) | ||
| }) | ||
| return r.key, r.err | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
|
|
||
| if res, ok := r.cache[storage]; ok { | ||
| return res.key, res.err | ||
| } | ||
|
|
||
| key, err := r.retrieveKeyOnce(storage) | ||
| r.cache[storage] = securityResult{key: key, err: err} | ||
| return key, err |
There was a problem hiding this comment.
SecurityCmdRetriever assumes r.cache is non-nil; if the retriever is constructed with a zero value (e.g., &SecurityCmdRetriever{}), the first cache write will panic. Consider lazily initializing the map in RetrieveKey when r.cache == nil to keep the exported type safe to use.
| func (r *SecurityCmdRetriever) RetrieveKey(storage, _ string) ([]byte, error) { | ||
| r.once.Do(func() { | ||
| r.key, r.err = r.retrieveKeyOnce(storage) | ||
| }) | ||
| return r.key, r.err | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
|
|
||
| if res, ok := r.cache[storage]; ok { | ||
| return res.key, res.err | ||
| } | ||
|
|
||
| key, err := r.retrieveKeyOnce(storage) | ||
| r.cache[storage] = securityResult{key: key, err: err} |
There was a problem hiding this comment.
SecurityCmdRetriever holds mu for the entire RetrieveKey call, including while running the external security command (which can block up to securityCmdTimeout). This serializes all key retrievals across different storages and can unnecessarily stall concurrent extractions; consider releasing the lock while executing the command and only locking to read/write the cache (or using a per-storage singleflight/once).
| // because macOS/Linux retrievers don't need it. | ||
| func (b *Browser) getMasterKey(session *filemanager.Session) ([]byte, error) { | ||
| if b.retriever == nil { | ||
| return nil, fmt.Errorf("key retriever not set for %s", b.cfg.Name) |
There was a problem hiding this comment.
With the new SetRetriever flow, getMasterKey returns an error when b.retriever is nil, but Extract currently just logs it at debug and continues. If a caller forgets to call SetRetriever and requests encrypted categories (passwords/cookies/cards), they’ll likely get silently empty results. Consider having Extract return an error (or at least a warning) when encrypted categories are requested and the retriever is unset/unavailable.
| return nil, fmt.Errorf("key retriever not set for %s", b.cfg.Name) | |
| msg := fmt.Sprintf("key retriever not set for %s: call SetRetriever before extracting encrypted categories", b.cfg.Name) | |
| fmt.Fprintln(os.Stderr, "warning:", msg) | |
| return nil, fmt.Errorf(msg) |
Summary
Closes #559
On macOS, when extracting multiple Chromium-based browsers (Chrome, Edge, Brave, etc.), each browser previously created its own
KeyRetrieverinstance. This caused:TerminalPasswordRetrieverasked for the login password once per browser, even though they all unlock the samelogin.keychain-dbGcoredumpRetrieverrangcoreon securityd once per browser, a very expensive operationSecurityCmdRetrievertriggered a macOS popup per browserThis PR lifts the
KeyRetrievercreation fromchromium.NewBrowsers()up topickFromConfigs(), so a single retriever instance is shared across all browsers.Changes
browser/browser.go: Create oneretrieverviaDefaultRetriever()and pass it to allnewBrowsers()callsbrowser/chromium/chromium.go: Acceptretrieveras a parameter instead of creating one internallyGcoredumpRetriever: Cache all keychain records (not just one key), look up per-storage on demandDecryptKeychain()→DecryptKeychainRecords(): Return all generic password records instead of a single storage-specific passwordTerminalPasswordRetriever: Generic prompt ("Enter macOS login password") since it's no longer browser-specific; added warning log on incorrect passwordSecurityCmdRetriever: Replacesync.Once(single key) withsync.Mutex+map(per-storage cache), since thesecurityCLI requires a storage name per queryWhy this approach
The keychain is a single database — unlocking it once gives access to all browser storage keys. The previous per-browser retriever design was correct for single-browser use but didn't account for the multi-browser extraction path. Sharing the retriever is harmless on Windows (DPAPI) and Linux (D-Bus) since those retrievers are stateless.
Test plan
go test ./...passesgo build ./cmd/hack-browser-data/succeeds