Login Flow Improvements and Fallback Handling

Review: PR #1366 — login: default to browser sign-in, add --device fallback

Verdict: Approve. Solid PR — clean structure, good test coverage at both unit and integration levels, careful comments, and the security fundamentals (PKCE S256, loopback-only listener, URL scheme validation, token validation before persist) are all in place. Unit tests pass locally. Two real-world UX gaps worth considering before or shortly after merge.

What it does

entire login now defaults to the RFC 8252 loopback authorization-code flow with PKCE (via auth-go v0.5.0's authcode package) when an interactive terminal is present, falling back to the device-code flow otherwise. --device forces the old flow. Both flows converge on a shared persistLogin tail (validate → keyring → contexts.json).

Strengths

• Good factoring. Extracting persistLogin instead of duplicating the validate/save/record tail, and the browserAuthFlow interface mirroring the existing deviceAuthClient pattern, keeps login.go consistent with itself. shouldUseBrowserLogin as a pure function with a table test is the right shape.
• Test design. The integration test's trick — openBrowser reports failure under test so the fallback URL lands on stdout, then the test plays the browser against the loopback callback — exercises the real listener, real PKCE (code_verifier asserted at the token endpoint), and real state round-trip without spawning anything. The interactive.UnderTest() guards in waitForEnter and openBrowser follow the documented project pattern.
• Housekeeping done. The "before merge" item (re-pin from the auth-go branch pseudo-version to tagged v0.5.0) is already done in go.mod/go.sum. Error wrapping preserves sentinels for errors.Is (verified by TestRunBrowserLogin_WaitError), and the //nolint:wrapcheck comments explain why.

Issues worth considering

1. SSH with a TTY gets a broken default (cmd/entire/cli/login.go:79). CanPromptInteractively() is true over ssh -t, so the browser flow is chosen — but the loopback listener binds 127.0.0.1 on the remote host, which the user's local browser can't reach. Even the printed fallback URL can't complete. The user's only recourse is Ctrl-C and --device. gh and gcloud detect this (e.g. SSH_CONNECTION/SSH_TTY env vars) and route to the code flow. Suggest adding an SSH check to shouldUseBrowserLogin, or at minimum mentioning --device in the "Waiting for sign-in..." context so a stuck user knows the escape hatch.

2. Wait has no deadline (login.go:184). The device flow bounds waiting via expires_in (capped at 15 min); the browser flow waits on the command context, which has no timeout — if the user closes the browser tab without completing, entire login hangs until Ctrl-C. A context.WithTimeout around flow.Wait (5–15 min) would match the device flow's behavior and most peer CLIs.

Minor / non-blocking

• No fallback when the listener can't start (login.go:80-83). If StartBrowserAuth fails (port bind, firewall), the command errors out rather than falling back to the device flow. Falling back here would be friendlier and costs little.
• TestLogin_BrowserFlow_SavesToken doesn't assert the saved token — it checks "Login complete." only. This matches the existing TestLogin_SavesTokenAfterApproval convention exactly, so it's fine as-is, but both names slightly overpromise; asserting the ENTIRE_TEST_AUTH_STORE_FILE contents would make them honest. Could be a follow-up touching both.
• waitForBrowserPrompt's 10s deadline only fires between reads (a ReadString block ignores it) — but this mirrors the pre-existing waitForLoginPrompt, and the pipe closes when the process exits, so it's not a new hazard.

Security notes

All good: PKCE with S256 (verified against prod discovery doc per the PR body), state round-tripped and validated by the authcode lib, openBrowser still rejects non-HTTP(S) URLs before the test guard, token iss/exp cross-checked before persisting, and the loopback redirect was verified registered server-side for any-port 127.0.0.1/callback. The full authorize URL (with PKCE challenge) deliberately isn't printed on the happy path — nice touch.

All done. Created branch login-browser-flow-hardening on top of login-loopback-browser-flow with three commits, one per fix:

1. 86c9baa83 — login: time out the browser sign-in wait after 5 minutes (review issue 2). runBrowserLogin now takes a waitTimeout and wraps the loopback wait in a context.WithTimeout (5 min, per your call). The clock starts after the Enter prompt so reading time isn't counted, the timeout error points at entire login --device as the escape hatch, and Ctrl-C is still reported as cancellation, not a timeout (there's a test pinning that distinction).

2. 4f8ab5e5d — login: fall back to the device flow in SSH sessions (review issue 1). New isSSHSession() checks the SSH_CONNECTION/SSH_CLIENT/SSH_TTY vars sshd sets, and shouldUseBrowserLogin now requires not-SSH. Users over SSH get a one-line stderr explanation ("SSH session detected; using device-code flow…") instead of a loopback URL their browser can't reach. This commit also extracts the flow choice from the cobra RunE into a testable runLoginAuto(...) that takes a startBrowser func and the environment facts as plain bools — the SSH/headless/--device message paths each have a unit test. One subtlety: the browser-flow integration test inherits os.Environ(), so it now blanks the SSH_* vars — otherwise running the test suite over SSH would silently flip it to the device flow.

3. 166393942 — login: fall back to the device flow when the loopback listener fails (the first nit). If StartBrowserAuth can't bind the listener, login warns on stderr with the bind error and continues with the device flow instead of erroring out.

Verification: mise run fmt + mise run lint pass, all new and existing login unit tests pass, the login integration tests (including the browser flow) pass, and a full mise run check exits 0. One caveat: the very first mise run check run had a single test failure somewhere outside the login code that I couldn't attribute (the output scrolled past), and two subsequent full test:ci runs were completely green — so it looks like an unrelated flake, but worth knowing it happened.

The branch is local only — say the word if you want it pushed / a PR opened against login-loopback-browser-flow.

Simplify pass complete — committed as 334df9885 on login-browser-flow-hardening. Full mise run check passes (exit 0).

Applied (4 findings, deduped from 4 agents):

• Bool plumbing → struct (simplification): runLoginAuto's three trailing bools became a loginFlowFacts struct with field comments, and shouldUseBrowserLogin takes it too. Call sites in tests went from false /* useDevice */, true /* canPrompt */, false /* ssh */ to loginFlowFacts{canPrompt: true}.
• Redundant /dev/tty probe (efficiency): my refactor had left runLogin calling interactive.CanPromptInteractively() again on the device path, after RunE had already probed once. runLogin now takes canPrompt as a parameter.
• SSH blanking centralized (altitude): blanking SSH_CONNECTION/SSH_CLIENT/SSH_TTY moved from the one browser-flow test into startLoginProcess, so every future login subprocess test is SSH-safe by default instead of relying on a per-test ritual — same idea as GitIsolatedEnv. extraEnv still appends last, so a test could set them deliberately.
• Test boilerplate (simplification): the startBrowser adapter closure shrank from 8 lines to a direct return client.StartBrowserAuth(ctx) (the typed-nil interface on error is harmless since callers check err first), and seven inline noopOpen lambdas collapsed into one shared noopOpenURL helper.

Skipped (3 findings):

• Move isSSHSession into the interactive package (reuse agent) — the altitude agent independently argued the opposite, and I agree with it: interactive answers "can we prompt?", which SSH doesn't affect (SSH sessions prompt fine). SSH-ness is a browser-reachability fact specific to login; nothing else in the repo needs it.
• Switch → if/else in runLoginAuto — the switch with one case per fallback reason (including the documented empty --device case) reads as a priority list; an if/else chain repeating !useDevice && would be no simpler.
• startBrowserStub returning a struct with a calls-getter instead of taking *int — more machinery, not less, for a five-line test stub.