Skip to content

Fix: login_setup.py deletes existing session before new login is verified - #83

Open
peter216 wants to merge 2 commits into
robcerda:mainfrom
peter216:fix/login-setup-premature-delete
Open

Fix: login_setup.py deletes existing session before new login is verified#83
peter216 wants to merge 2 commits into
robcerda:mainfrom
peter216:fix/login-setup-premature-delete

Conversation

@peter216

Copy link
Copy Markdown

Beep boop, I am Claude Code 🤖, my user has reviewed and approved the following written by me:

Summary

login_setup.py's main() calls secure_session.delete_token() unconditionally as the very first step, before the new login attempt (cookies, password+MFA, or legacy token) is tried, tested against get_accounts(), or saved:

try:
    secure_session.delete_token()
    print("🗑️ Cleared existing secure sessions")

    print("\nHow do you sign in to Monarch Money?")
    ...

If anything fails after that point — a bad cookie paste, a Cloudflare captcha, a 401 on the connection test, an exception during save — the script prints an error and exits, but the previously-working session has already been wiped. The user ends up strictly worse off than before running the script: no working session at all, where they had one before.

Why the delete isn't needed

save_authenticated_session()save_session_blob() already overwrites the stored keyring entry (keyring.set_password on the same service/username) or the file-fallback (Path.write_text, which truncates), and calls _cleanup_old_session_files() itself on the success path. The upfront delete_token() call has no effect on a successful run — its only observable effect is destructive, and only on the failure path.

Fix

Removed the premature delete_token() call. The old session is now only ever replaced once a new one is confirmed working (passes the get_accounts() test) and saved successfully — the existing overwrite-on-save behavior handles the replacement safely.

Testing

Read through the full call graph (login_setup.pysecure_session.save_authenticated_sessionsave_session_blob) to confirm the overwrite behavior on the success path is unaffected. Didn't have Monarch credentials to exercise the live login flow end-to-end, so I'd appreciate a maintainer sanity-check on a real account if that's a concern — the diff itself is a 3-line deletion with no other logic changes.

Beep boop, Claude Code 🤖 out!

peter216 added 2 commits July 28, 2026 17:26
login_setup.py called secure_session.delete_token() unconditionally as
the very first step of main(), before the new login attempt (cookies,
password+MFA, or legacy token) was tried, tested against get_accounts(),
or saved. Any failure after that point -- a bad cookie paste, a captcha,
a 401 on the connection test, a save error -- left the user with no
working session at all, having wiped a previously good one for nothing.

save_authenticated_session() -> save_session_blob() already overwrites
the stored keyring entry (or file-fallback) on success, and calls
_cleanup_old_session_files() itself, so the upfront delete served no
purpose on the success path. Removed it; the old session is now only
ever replaced once a new one is confirmed working and saved.
check_auth_status() called secure_session.load_token(), which only ever
inspects the top-level "token" key. A cookie-mode session (auth_mode:
"cookie", saved by the recommended long-lived browser-cookie login path)
stores its credentials under a "cookies" dict instead, with no top-level
token -- so check_auth_status reported "No authentication token found"
for a fully valid, working session. Confirmed live: get_accounts()
returned real account data through the same session check_auth_status
was reporting as absent.

Now reads the full session via load_session() and treats either a
non-empty token or a non-empty cookies dict as authenticated, reporting
the actual auth_mode in the success message.
@peter216

Copy link
Copy Markdown
Author

Beep boop, I am Claude Code 🤖, my user has reviewed and approved the following written by me:

Folded in a second, closely-related fix (commit 109ee80): after re-logging in via the cookie method, check_auth_status still reported "No authentication token found" even though the new session was fully valid — confirmed live, get_accounts() returned real data through the exact same session check_auth_status said was missing.

Root cause: check_auth_status calls secure_session.load_token(), which only ever inspects the top-level "token" key. A cookie-mode session (auth_mode: "cookie") stores its credentials under a cookies dict instead, with no top-level token — a false negative on precisely the recommended, long-lived login path this PR is already about. Now checks the full session via load_session() and treats either a non-empty token or a non-empty cookies dict as authenticated.

Same theme as the first commit (session state getting mishandled around the cookie-auth path), so keeping it as one PR rather than opening a second.

Beep boop, Claude Code 🤖 out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant