feat: Fix keychain issues and add fs fallback#10
Open
jpage-godaddy wants to merge 23 commits into
Open
Conversation
- Add missing features for `keyring` to make it actually work instead of being a no-op (grr) - Allow option for storing token in a file as a fallback for keychain issues (disabled by default) - Add ability to get debug logging
There was a problem hiding this comment.
Pull request overview
This PR aims to make PKCE auth token persistence reliable by fixing keychain backend configuration and adding an opt-in file-based fallback for environments where the system keychain is unavailable, along with additional debug/warn logging around keychain operations.
Changes:
- Add
keyringOS-specific feature flags so keychain storage is functional on Linux/macOS/Windows. - Add an opt-in file fallback for token storage when keychain operations fail.
- Add tracing logs around keychain read/write failures and fallback usage.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/auth/pkce.rs |
Implements file fallback storage/removal and adds keychain error logging. |
Cargo.toml |
Configures keyring with OS-specific backend features. |
Cargo.lock |
Updates lockfile for new/changed transitive dependencies pulled in by keyring features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- config_base_dir: add .filter(|p| p.is_absolute()) so a relative XDG_CONFIG_HOME/APPDATA/HOME does not silently place credentials relative to the current working directory - delete_token_from_keychain: handle JoinError with a WARN log, consistent with the read/write paths - Add test: credential_file_path_rejects_relative_base_dir Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- save_token_to_keychain: change "keychain is unavailable" to "failed to save token to keychain" so the message is accurate for all failure modes (permissions, locked, truly absent) - Add file_fallback_round_trip_write_then_read: exercises write_token_file_blocking + load_token_from_file together - Add file_fallback_invalid_json_returns_none: verifies corrupted credential files are handled gracefully Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ponents is_safe_path_component now also rejects: - trailing '.' or ' ' (valid on Unix but rejected by Windows) - Windows reserved device names (CON, NUL, COM1-9, LPT1-9 with or without extension) — opening e.g. NUL.json on Windows writes to the null device rather than a file Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If std::fs::rename fails the token data would otherwise sit in an orphaned *.tmp file. Now best-effort deletes the temp file before returning the error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- save_token_to_keychain: when keychain write succeeds, best-effort delete any leftover file-fallback token (NotFound is silently ignored; other errors logged at debug) - config_base_dir: update comment to accurately describe the non-Windows fallback order (HOME/.config first, then APPDATA as last resort) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- delete_token_from_keychain: replace drop(delete_credential()) with a match that logs WARN on unexpected errors (NoEntry is silently ignored — token already gone is fine during logout) - load_token_from_file: best-effort delete the file after logging invalid JSON so the system self-heals instead of forcing re-auth on every subsequent run Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, fix CI doc tests - delete_token_from_keychain: clone service before closure so the JoinError warning includes the service name; log WARN when Entry::new fails (consistent with read/write helpers) - load_token_from_keychain: best-effort delete the keychain entry when its JSON is corrupt, preventing repeated warnings on every run - ci.yml: add --features pkce-auth to the Doc Tests step so the pkce module's no_run example is compile-checked in CI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
keyringto make it actually work instead of being a no-op (grr)