security: clear stale auth state at login entry - #411
Merged
Conversation
Contributor
WalkthroughLogin now clears stale authentication and MFA/session state before credential attempts while preserving the configured token-store path. Tests verify cleanup ordering, fresh web JWT usage, failed-login cleanup, and path preservation. ChangesAuthentication cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@garminconnect/client.py`:
- Around line 417-424: Update every cleanup call within login’s fallback flow,
including the MFA and strategy token-rejection paths in
garminconnect/client.py:417-424, to pass keep_tokenstore_path=True so later
strategies preserve the configured token-store path. In
tests/test_garmin_unit.py:738-743, update the second expected cleanup call to
call(keep_tokenstore_path=True) and verify that a rejected strategy followed by
a successful strategy retains the path.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 25cf1782-4bc1-4307-93fb-557f8749e9ec
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
cyberjunky
force-pushed
the
security/login-clear-stale-auth
branch
from
August 10, 2026 13:06
887ec40 to
480d747
Compare
login()'s only entry guard checked _mfa_pending; nothing cleared prior auth state. A Client carrying an old di_token (previous login or loaded token store) hit two defects: 1. Failed login didn't clean up: strategy exhaustion raised without touching tokens, so login() throws but is_authenticated stays True with the old token intact. 2. Web login didn't displace di_token: get_api_headers() prefers di_token over jwt_web and web strategies only set jwt_web, so the client kept sending the old bearer - and _verify_token() validated that same stale token, blessing the login as the wrong identity. Under Client-reuse patterns this is identity confusion across accounts; even single-user, a failed re-login leaves the app believing it is authenticated with dead credentials. login() now calls _clear_auth_state(keep_tokenstore_path=True) at entry. The new keyword preserves the persistence target the Garmin wrapper sets before delegating, so token refresh keeps dumping to the same store; logout behavior is unchanged.
cyberjunky
force-pushed
the
security/login-clear-stale-auth
branch
from
August 10, 2026 13:18
480d747 to
e7baf99
Compare
Addresses CodeRabbit review on PR #411: the MFA token-rejection and strategy token-rejection paths called _clear_auth_state() without keep_tokenstore_path=True, wiping the path the wrapper set before login. A rejected strategy followed by a successful one then left the client unable to persist refreshed tokens to the configured store.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@garminconnect/client.py`:
- Around line 308-321: Update the token-rejection branch in resume_login() to
call _clear_auth_state(keep_tokenstore_path=True), preserving the configured
token-store path during deferred MFA failures. Add a regression test covering
login(return_on_mfa=True), subsequent MFA token rejection, and a later
successful login that persists refreshed tokens.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d4317cec-4749-410d-af70-d7e5cf2b0a71
📒 Files selected for processing (2)
garminconnect/client.pytests/test_garmin_unit.py
Comment on lines
+308
to
+321
| def _clear_auth_state(self, *, keep_tokenstore_path: bool = False) -> None: | ||
| """Wipe all in-memory auth tokens and session state so the next login starts clean. | ||
|
|
||
| ``keep_tokenstore_path`` preserves the persistence target: login() sets | ||
| it via the Garmin wrapper *before* the credential flow runs, and a | ||
| fresh login should keep persisting to the same store. | ||
| """ | ||
| self.di_token = None | ||
| self.di_refresh_token = None | ||
| self.di_client_id = None | ||
| self.jwt_web = None | ||
| self.csrf_token = None | ||
| self._tokenstore_path = None | ||
| if not keep_tokenstore_path: | ||
| self._tokenstore_path = None |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the token-store path after deferred MFA token rejection.
resume_login() handles MFA attempts created by login(return_on_mfa=True). At Line 1503, it calls _clear_auth_state() without keep_tokenstore_path=True. This clears the configured persistence target when _verify_token() rejects the completed MFA token.
Use keep_tokenstore_path=True in that branch. Add a regression test for deferred MFA followed by token rejection. Otherwise, a later successful login on the same Client cannot persist refreshed tokens.
Proposed fix
if self.verify_login and not self._verify_token():
- self._clear_auth_state()
+ self._clear_auth_state(keep_tokenstore_path=True)
raise GarminConnectConnectionError(
"token rejected by API tier after MFA"
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@garminconnect/client.py` around lines 308 - 321, Update the token-rejection
branch in resume_login() to call _clear_auth_state(keep_tokenstore_path=True),
preserving the configured token-store path during deferred MFA failures. Add a
regression test covering login(return_on_mfa=True), subsequent MFA token
rejection, and a later successful login that persists refreshed tokens.
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
1 participant
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
From the external security audit (medium severity).
login()never cleared previous auth state — its only entry guard checked_mfa_pending. AClientcarrying an olddi_token(from a previous login on the same instance, or a loaded token store) hit two defects:login()throws butis_authenticatedstaysTrueandget_api_headers()keeps returningBearer <old token>.di_token.get_api_headers()unconditionally prefersdi_tokenoverjwt_web, and the web strategies only setjwt_web— so after a successful web login the fresh token was silently ignored and requests went out with the stale bearer. Worse, withverify_login=True(default)_verify_token()validated that same stale token, blessing the login as the wrong identity.Impact
Identity confusion / confused-deputy under Client-reuse patterns (e.g. a multi-tenant server caching Client objects — requests run as the previous user). Even single-user: a failed re-login leaves the app believing it's authenticated with dead credentials.
Fix
login()now calls_clear_auth_state(keep_tokenstore_path=True)at entry (after the_mfa_pendingguard): failed logins end unauthenticated, and a web-strategyjwt_webcan no longer be shadowed by a staledi_token._clear_auth_state()gainedkeep_tokenstore_path: bool = False— theGarminwrapper sets_tokenstore_pathbefore delegating toclient.login(), and wiping it would silently break token persistence on refresh.logout()behavior is unchanged (path still cleared).Tests
New
TestLoginClearsStaleAuth:test_failed_login_clears_stale_token— all strategies fail →is_authenticatedFalse,get_api_headers()raisestest_web_login_uses_fresh_jwt_not_stale_di_token— staledi_tokencleared; headers carry the freshJWT_WEBcookietest_login_preserves_tokenstore_path— persistence target survives loginUpdated
test_mfa_token_rejection_falls_through_to_next_strategyfor the new entry-clear call. 182 passed; only the 3 known Windows-symlink-privilege failures remain (pre-existing, unrelated).Summary by CodeRabbit