fix(keychain): reject null collection path on linux#538
Conversation
On headless hosts the secret service may be running without a usable
collection: no 'login' collection (PAM only creates/unlocks it on a
graphical login) and no collection assigned to the 'default' alias. In
that case ReadAlias("default") returns the null object path "/".
getDefaultCollection previously only guarded with ObjectPath.IsValid(),
which returns true for "/" since it is syntactically valid. The null
path was therefore returned as the collection and every subsequent
D-Bus call failed with the cryptic 'Object does not exist at path "/"'.
Reject the null path explicitly and return a typed
errNoDefaultCollection so callers can branch to a fallback and users get
a clear error.
Refs #56
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
The core fix is correct: nullObjectPath = dbus.ObjectPath("/") is well-typed, the guard is placed after IsValid() (which passes for "/"), and errNoDefaultCollection gives callers a named error to branch on. Two design concerns were identified by static analysis (verifier inconclusive — findings are unverified draft hypotheses).
| // default collection (no 'login' collection and no collection assigned to the | ||
| // 'default' alias). This typically happens on headless hosts where the keyring | ||
| // has not been initialized. | ||
| var errNoDefaultCollection = errors.New("no default keychain collection available") |
There was a problem hiding this comment.
[MEDIUM] errNoDefaultCollection is unexported, preventing out-of-package error detection
The new errNoDefaultCollection sentinel is package-private. All public store methods (Get, Save, Delete, GetAllMetadata, Filter) propagate it directly to callers without wrapping. Any code outside the keychain package — such as an orchestration layer that wants to fall back gracefully on headless hosts — cannot use errors.Is to distinguish "no keychain infrastructure available" from other I/O errors. Since the PR's stated motivation is exactly headless Linux deployments where this condition is expected and routine, making this sentinel exported (or wrapping it in an exported type) would allow callers to handle it programmatically rather than resorting to fragile message-string comparisons.
Note: automated verifier was inconclusive; this finding is from static draft analysis.
| // assigned to the 'default' alias. This is common on headless hosts where | ||
| // neither the 'login' collection nor a 'default' alias has been set up. | ||
| if defaultKeychainObjectPath == nullObjectPath { | ||
| return "", errNoDefaultCollection |
There was a problem hiding this comment.
[MEDIUM] Delete returns errNoDefaultCollection on headless hosts instead of nil, breaking idempotent-delete semantics
On a headless host where no default collection exists, Delete (line 138) propagates errNoDefaultCollection for every call — even when the credential being deleted is guaranteed to be absent (there is no collection to store it in). The existing tests document that deleting an absent credential should return nil. Consider special-casing errNoDefaultCollection inside Delete (return nil) while still propagating it as an error from read and write-new-item operations (Get, Save, GetAllMetadata, Filter).
Note: automated verifier was inconclusive; this finding is from static draft analysis.
Extract the collection-selection logic from getDefaultCollection into a pure resolveDefaultCollection helper so it can be unit tested without a live secret service over dbus. Add table-driven tests covering: login preference, fallback to the default alias, rejection of the null path "/" from an unassigned alias (the headless-host case), and rejection of a syntactically invalid path. Refs #56 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
On headless hosts the secret service may be running without a usable collection: no 'login' collection (PAM only creates/unlocks it on a graphical login) and no collection assigned to the 'default' alias. In that case ReadAlias("default") returns the null object path "/".
getDefaultCollection previously only guarded with ObjectPath.IsValid(), which returns true for "/" since it is syntactically valid. The null path was therefore returned as the collection and every subsequent D-Bus call failed with the cryptic 'Object does not exist at path "/"'.
Reject the null path explicitly and return a typed errNoDefaultCollection so callers can branch to a fallback and users get a clear error.
Refs #56