Fix login URL handling, add Ctrl+C support, add logout command#740
Fix login URL handling, add Ctrl+C support, add logout command#740matthiaswenz wants to merge 5 commits intomainfrom
Conversation
- Show clean verification_uri to the user, but open verification_uri_complete (with code pre-filled) in the browser - Make waitForEnter context-aware so Ctrl+C exits cleanly instead of leaving the process stuck on a blocking tty read Fixes regression from #728 where the browser-opened URL lost the auth code parameter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1518a16ea037
Adds `entire logout` which deletes the auth token from the OS keyring, fully reversing `entire login`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 43f1c3795354
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| select { | ||
| case <-ctx.Done(): | ||
| // Close tty to unblock the reading goroutine. | ||
| _ = tty.Close() |
There was a problem hiding this comment.
Double close of tty file descriptor
Medium Severity
In waitForEnter, when the context is cancelled, tty.Close() is called explicitly on line 200 to unblock the reader goroutine, but defer tty.Close() on line 188 will also run when the function returns. This double-close of tty is a correctness bug — after the first close releases the file descriptor, another goroutine could reuse that FD number, and the deferred close would then silently close an unrelated file.
Additional Locations (1)
There was a problem hiding this comment.
Pull request overview
Fixes entire login’s device-auth URL handling and interrupt behavior, and adds a logout command to remove stored credentials from the keyring.
Changes:
- Print the clean
verification_uribut openverification_uri_completein the browser during login. - Make the “Press Enter…” prompt context-aware to respond to Ctrl+C.
- Add
entire logoutto delete the stored token from the OS keyring (plus basic command registration tests).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/entire/cli/root.go | Registers the new logout subcommand on the root command. |
| cmd/entire/cli/logout.go | Implements entire logout by deleting the token from the keyring for the current base URL. |
| cmd/entire/cli/logout_test.go | Adds tests for logout output/registration/Short description (currently non-hermetic). |
| cmd/entire/cli/login.go | Splits display vs browser URLs; makes the enter prompt cancellable via context. |
When the browser fails to open, print the full verification URL (with code pre-filled) so the user can copy-paste it directly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 6e7deeb97fd7
Extract runLogout with a tokenDeleter interface so tests use a mock instead of the real OS keyring, preventing failures on CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 1d26a3e7f936
Showing one URL but opening another is a phishing vector — an attacker who can tamper with the device auth response could set verification_uri_complete to a lookalike domain while the clean verification_uri looks legitimate in the terminal. Always show and open the same URL. The server can handle pre-filling the device code on its end. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Entire-Checkpoint: d9afebaaf0f8
Summary
verification_uri: Never openverification_uri_complete— showing one URL but opening another is a phishing vector (an attacker who can tamper with the device auth response could setverification_uri_completeto a lookalike domain). Fixes regression from Use cleaner base URL in login output #728.waitForEntercalledtty.Close()on cancellation while adefer tty.Close()also ran on return — after the first close releases the FD, another goroutine could reuse that FD number and the deferred close would silently close an unrelated fileentire logout: Removes stored credentials from the OS keyring, fully reversingentire loginTest plan
mise run test:ci)entire login, verify the same URL is shown and opened in browserentire login, hit Ctrl+C at the prompt — verify clean exitentire loginwith browser unavailable — verify URL is printed for copy-pasteentire logout— verify "Logged out." message and token removed from keyring🤖 Generated with Claude Code