feat: system keychain credential storage with filesystem fallback#4
feat: system keychain credential storage with filesystem fallback#4bowlofarugula wants to merge 2 commits intomasterfrom
Conversation
Uses the keyring library to store OAuth tokens in the OS keychain (macOS Keychain, GNOME Keyring/KDE Wallet, Windows Credential Locker), falling back to ~/.murl/credentials/ JSON files when keyring is not installed or no secure backend is available. Mirrors the gh CLI approach. Install with: pip install mcp-curl[keychain] Migration is automatic — existing file credentials are still readable, and re-saving promotes them to the keychain. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request introduces a keyring-based credential storage backend to the token management system while preserving the existing filesystem backend. The implementation includes a priority-based lookup that attempts to retrieve credentials from the system keychain first, then falls back to the filesystem. The public API remains unchanged with identical function signatures. New test coverage includes filesystem and keyring-specific test classes, migration scenarios from filesystem to keychain, and fallback behavior when keyring operations fail. A new optional dependency Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@murl/token_store.py`:
- Around line 109-113: The _file_delete helper currently raises if path.unlink()
fails, turning a transient filesystem error into a user-visible auth failure
when credentials were already written to the keychain; change _file_delete to
perform legacy-file cleanup as best-effort by catching OSError (and subclasses
like PermissionError) around path.unlink(), suppressing the exception
(optionally logging a debug/warning via the module logger) so save_credentials()
does not fail when unlink() transiently errors; locate and update the
_file_delete function to wrap unlink() in a try/except that only swallows
filesystem-related exceptions.
- Around line 93-105: The file write in _file_set currently creates the
credential file with the process umask and only calls chmod after writing,
leaving a race where the file can be created with weaker permissions; fix by
opening the file with os.open using flags os.O_WRONLY | os.O_CREAT | os.O_TRUNC
and mode 0o600, wrap the returned fd with os.fdopen to write JSON (json.dump) so
the file is created atomically with 0600; ensure CREDENTIALS_DIR is created with
mode 0o700 (mkdir(..., mode=0o700)) and keep the existing try/except chmod
fallback for CREDENTIALS_DIR if desired, but remove reliance on a post-write
chmod of path (or keep it as a no-op safety fallback).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e23d9a64-04bf-4454-9fdc-ada90466195b
📒 Files selected for processing (3)
murl/token_store.pypyproject.tomltests/test_auth.py
- Create credential files with 0600 from the start via os.open() to avoid a brief window where tokens are world-readable under permissive umask - Make _file_delete best-effort so transient unlink errors don't surface as auth failures when creds are already safely in the keychain Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed both CodeRabbit findings in 37fc9fa:
|
turlockmike
left a comment
turlockmike
left a comment
There was a problem hiding this comment.
Two-lens review: security, test quality.
Overall: Right design direction — keychain-first with file fallback mirrors gh. The critical improvement: _file_set now uses os.open() with 0o600 from creation, eliminating the old open() + chmod() TOCTOU race. Several items to address.
Key findings:
-
Silent security tier demotion — Keyring failures (
except Exception: return None) silently fall back to file storage without informing the user. If an attacker or faulty environment causes the keychain read to throw, the code uses the weaker backend without any signal. At minimum, log a warning; ideally, distinguish "not found" from "backend error." See inline. -
clear_credentialsdoesn't guarantee deletion —_keyring_deleteswallows all exceptions. A user who invokes "logout" believes their token is gone; if keychain deletion silently fails, the token remains accessible. Should raise if both deletions fail. See inline. -
Test gaps — Missing tests for: (a) fallback round-trip (save via file when keyring fails, then
get_credentialsretrieves it), (b) corrupt JSON in credential file (json.JSONDecodeErrorhandler is untested), (c) migration-on-get vs migration-on-save distinction.
Additional observations:
_keyring_available()called on every credential operation with no caching — creates a TOCTOU window between get and save calls where the backend can change stateserver_urlstored in plaintext inside the credential blob in both backends — not a regression from the file-only design, but worth documenting_mock_keyringcreates an unusedMagicMock()object that adds confusion — clean up or add a comment- Test names (
test_roundtrip,test_clear) describe the operation, not the behavioral claim — better:test_credential_stored_in_keychain_not_on_disk
What's good: The _file_set rewrite (using os.open() with 0o600 as the creation mode) is the most important fix and it's done correctly — the file is never world-readable even for a millisecond. SHA-256 of the URL as storage key avoids raw URL as keychain username. The in-memory keyring mock in tests is well-built. test_keyring_failure_falls_back_to_file and test_migration_from_file are exactly the high-value paths most PRs would omit.
— Mike's AI Clone 🧬
| if data is None: | ||
| return None | ||
| return json.loads(data) | ||
| except Exception: |
There was a problem hiding this comment.
[Security] Warning — Silent Security Tier Demotion
_keyring_get catches all exceptions and returns None, which get_credentials treats as "not found" and falls through to the file backend. If a keychain read throws (D-Bus disruption on Linux, corrupt entry, signal), the code silently uses the weaker backend without informing the caller.
On a system where migration is incomplete (file still exists alongside keychain), this means credentials come from the weaker backend without any signal. At minimum, log a warning when a keyring operation throws. The same pattern applies to _keyring_set.
| _file_set(key, data_to_save) | ||
|
|
||
|
|
||
| def clear_credentials(server_url: str) -> None: |
There was a problem hiding this comment.
[Security] Warning — clear_credentials Doesn't Guarantee Deletion
_keyring_delete swallows all exceptions including PasswordDeleteError. clear_credentials calls both backends and returns with no indication of whether either deletion succeeded.
A user who invokes "logout" believes their token is gone. If keychain deletion silently fails, the token remains accessible to any process that can read the keychain. At minimum, raise an exception if both deletions fail so the caller can surface an error.
2 participants
Summary
keyringlibrary, mirroring theghCLI approach~/.murl/credentials/*.jsonfiles when keyring is not installed or no secure backend is availablepip install mcp-curl[keychain]Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
pip install murl[keychain].Tests