Skip to content

Add CLI auth code fallback#41

Open
cloud9c wants to merge 9 commits into
mainfrom
kire5030/wvdsh-1727-cli-auth-codes
Open

Add CLI auth code fallback#41
cloud9c wants to merge 9 commits into
mainfrom
kire5030/wvdsh-1727-cli-auth-codes

Conversation

@cloud9c

@cloud9c cloud9c commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Switch browser login to PKCE-bound one-time auth codes
  • Start a localhost callback server for the happy path
  • Print the manual sign-in URL without the broken c to copy shortcut
  • Start the paste-code fallback prompt only after a short delay if the callback has not completed
  • Suppress update checks during auth login so prompts are not interrupted

Companion Wavedash PR: https://github.com/wvdsh/wavedash/pull/809

QA

  • SITE_HOST=wavedash.lvh.me CONVEX_HTTP_URL=https://adjoining-schnauzer-809.convex.site PLAYSITE_HOST=wavedash.localtest.me cargo test
  • rustfmt --check --edition 2021 src/auth.rs

Note

High Risk
Rewrites CLI authentication (PKCE, code redemption, localhost callback) and changes how API keys are obtained—security-critical behavior that must stay aligned with the companion server PR.

Overview
Browser login no longer receives the API key on the redirect. It now uses PKCE (random state + verifier/challenge), a localhost /callback server (replacing tiny_http), and redeems a one-time code via POST /cli/auth/redeem.

If the callback fails or takes too long, the CLI shows a manual sign-in URL (without the broken copy shortcut) and, after a short delay, a paste-code prompt; invalid pasted codes retry in the terminal.

wavedash auth login gains --token-stdin, refuses browser login in CI or non-interactive stdin (with guidance to use env/stdin), and background update checks are skipped during login so prompts are not interrupted.

Unit tests cover PKCE S256 and callback query parsing.

Reviewed by Cursor Bugbot for commit 9c79eaf. Bugbot is set up for automated code reviews on this repo. Configure here.

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

Claude finished @cloud9c's task in 3m 57s —— View job


Code Review — Add CLI auth code fallback

Reviewed the full diff against origin/main (auth flow, update-check suppression, and the --json/scripted surface added across the other commands). Overall this is a solid, well-structured change — the PKCE-bound one-time code exchange, random 32-byte state, S256 challenge, and strict state validation on the callback are all a real security improvement over the previous api_key-in-query-string flow. The tests for the PKCE derivation and callback parsing are a nice touch. Nothing here blocks merge; findings below are mostly UX/robustness.

Findings (posted inline)

  1. src/auth.rs:397 — A blank line cancels the entire login. In the paste-code fallback, an empty input line trims to "" and bail!s, which propagates as AuthInput::Error and aborts auth instead of re-prompting. Since the reader loops, returning Ok(None) on empty interactive input would re-prompt; only EOF should cancel. (suggestion attached inline)

  2. src/auth.rs:392 — Leaked stdin reader thread holds the global stdin lock. After the 3s timeout, a reader thread parks on io::stdin().lock().read_line. If the browser callback then succeeds, login_with_browser returns with that thread still holding the process-wide stdin mutex. Harmless today (process exits after auth login), but a latent deadlock if browser login is ever reused inside a longer interactive flow.

  3. src/main.rs:465is_browser_login_unavailable() only checks CI. Headless Docker/SSH/cron without CI set will attempt browser login; xdg-open often returns Ok even when nothing opens, so the user waits the full 3s before the paste prompt. Acceptable given the fallback — flagging to confirm intent and suggesting SSH_CONNECTION / non-tty stdout as extra signals.

Smaller notes (not posted inline)

  • src/auth.rs:401 — A pasted code exactly equal to c (case-insensitive) is intercepted as the "copy URL" command. Vanishingly unlikely given the token format, but worth being aware of.
  • src/init.rs / is_interactive() — Passing --force (or --json) alone routes wavedash init to the scripted path, which then fails validation demanding --team-id/--game-title. A user wanting an interactive re-init with overwrite can't use --force for that (the interactive path already prompts to overwrite), so --force is effectively scripted-only. Minor, but the coupling of force/json into is_interactive() is a little surprising.
  • JSON hygiene — Good call gating verbose/progress off under --json in builds.rs/uploader.rs and suppressing the update check for command_outputs_json. Note the update notification is already eprintln! (stderr), so stdout JSON stays clean either way. 👍

Not verified

I did not compile or run the suite — the QA steps require Doppler-managed env vars (SITE_HOST/CONVEX_HTTP_URL/PLAYSITE_HOST) that aren't available in this review environment. The two new unit tests look correct by inspection (the RFC 7636 S256 vector and the state-match/mismatch cases).
· branch kire5030/wvdsh-1727-cli-auth-codes

@cloud9c cloud9c marked this pull request as draft July 6, 2026 22:06
Comment thread src/auth.rs Outdated
Comment thread src/auth.rs
Comment thread src/main.rs Outdated
Comment thread src/init.rs Outdated
Comment thread src/auth.rs
Comment thread src/auth.rs Outdated
@cloud9c cloud9c force-pushed the kire5030/wvdsh-1727-cli-auth-codes branch from a9528c1 to 282b16b Compare July 7, 2026 16:58
@cloud9c

cloud9c commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 282b16b. Configure here.

@cloud9c

cloud9c commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread src/auth.rs
@cloud9c

cloud9c commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread src/auth.rs
Comment thread src/stats.rs Outdated
@cloud9c

cloud9c commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread src/auth.rs
@cloud9c

cloud9c commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread src/auth.rs
Comment thread src/auth.rs
Comment thread src/auth.rs
@cloud9c cloud9c marked this pull request as ready for review July 7, 2026 18:08
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @cloud9c's task in 3m 15s —— View job


Code Review — Add CLI auth code fallback

Re-reviewed the current branch (through 4ecac7a) against origin/main, focusing on the auth-code flow and the changes made in response to the earlier Claude and Bugbot rounds. The prior findings I raised have been addressed well, and the earlier Bugbot items are either fixed or intentional-with-rationale. One minor UX inconsistency remains (posted inline); nothing here blocks merge.

  • Read the full diff against origin/main
  • Review src/auth.rs (PKCE, callback server, redeem, fallback loop)
  • Review the --json/scripted surface (main.rs, init.rs, stats.rs, builds.rs, uploader.rs, publish.rs, achievements.rs)
  • Post findings

Verified fixes from earlier rounds ✅

  • Blank-line no longer aborts loginread_auth_code returns Ok(None) on empty input (auth.rs:324) and spawn_auth_code_reader loops on Ok(None), so a stray Enter re-prompts; only EOF cancels. 👍
  • Non-tty stdin fails the preflightis_browser_login_unavailable() now also checks !stdin().is_terminal() (main.rs:567), so headless/piped auth login bails with an actionable message instead of spawning a blocking reader or cancelling before the callback.
  • Callback-failure recovery is immediate — after a failed callback redeem the loop spawns the paste reader right away (auth.rs:475-481) rather than waiting on the 3s timer.
  • stat update no longer requires a parseable body — non-JSON mode only checks status; JSON mode parses (stats.rs:91-95).
  • Redeem is bounded — the browser connection can't hang indefinitely; redeem_auth_code carries a 10s timeout (auth.rs:405-406).
  • JSON hygieneverbose/progress correctly gated off under --json in builds.rs/uploader.rs, and the update check is suppressed for JSON output and during auth login (stdout stays clean).

Finding (posted inline)

  1. src/init.rs:79--force alone routes wavedash init to the scripted path and fails. is_interactive() returns false whenever force is set, so wavedash init --force (without team/game flags) dispatches to handle_init_scripted and immediately bails demanding --team-id. The interactive path already prompts to overwrite, so --force is effectively scripted-only. Suggest dropping !self.force from is_interactive() so --force only governs overwrite. (suggestion attached inline)

Smaller notes (not blocking)

  • Security review of the core change is positive. PKCE S256 with a 32-byte random verifier and state (ring::SystemRandom), strict state validation on the callback (auth.rs:225), localhost-only listener on an ephemeral port, and code+state+verifier exchanged server-side at /cli/auth/redeem — a genuine improvement over the previous api-key-in-query flow. Unit tests cover the RFC 7636 S256 vector and the state match/mismatch cases.
  • Leaked threads on the happy path (acknowledged, acceptable today). After a successful paste-code or callback login, the other worker can remain parked — the callback-server thread blocking on listener.incoming(), or the stdin reader holding the process-wide stdin lock. Harmless because auth login exits immediately, but a latent hazard if login_with_browser is ever composed into a longer-lived interactive flow. Worth a one-line comment noting the assumption.
  • redeem blocks the select! loop for up to 10s while a callback code is being exchanged, during which Ctrl-C is inert. Bounded and intentional per the discussion; flagging only so it stays a conscious tradeoff.
  • SSH/manual-code users wait ~3s for the paste prompt — intentional per your note (keeps Paste code here if prompted > from showing during a normal successful browser callback).

Not verified

I did not compile or run the suite — the QA steps require Doppler-managed env vars (SITE_HOST/CONVEX_HTTP_URL/PLAYSITE_HOST) that aren't available in this environment. Findings are from inspection.
· branch kire5030/wvdsh-1727-cli-auth-codes

Comment thread src/init.rs Outdated
&& self.upload_dir.is_none()
&& self.engine.is_none()
&& self.engine_version.is_none()
&& !self.force

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--force alone silently routes wavedash init to the scripted (non-interactive) path. Because is_interactive() returns false whenever force is set, wavedash init --force (with no --team-*/--game-* flags) is dispatched to handle_init_scripted, which immediately bail!s with "Non-interactive init requires --team-id …".

The interactive path already prompts to overwrite an existing wavedash.toml, so a user who just wants to re-init interactively and overwrite has no way to use --force for that — it's effectively scripted-only. Consider dropping !self.force from is_interactive() so --force only controls overwrite behavior, and let the presence of the team/game flags (or --json) be what selects the scripted path.

Suggested change
&& !self.force
&& self.engine_version.is_none()
&& !self.json

Fix this →

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @cloud9c's task in 3m 57s —— View job


Code Review — Add CLI auth code fallback

Re-reviewed the current branch (through 9c79eaf) against origin/main. The last commit (Remove unrelated changes from auth PR) scoped this PR down to 4 filesCargo.toml, Cargo.lock, src/auth.rs, src/main.rs. The --json/scripted surface, init.rs, and stats.rs changes described in the PR body are no longer part of this diff, so the earlier findings against those files (the --force init routing, stat update body parsing, etc.) are moot for this PR.

  • Read the full diff against origin/main
  • Review src/auth.rs (PKCE, callback, redeem, fallback loop)
  • Review src/main.rs (preflight, --token-stdin, update-check suppression)
  • Post findings

Summary

This is a clean, well-structured change and a genuine security improvement over the previous api_key-in-query-string flow. Nothing here blocks merge.

Verified good ✅

  • PKCE + one-time codes. 32-byte random state and code_verifier via ring::SystemRandom, S256 challenge, strict state validation on the callback (auth.rs:250-252), localhost-only ephemeral-port listener, and code+state+verifier exchanged server-side at /cli/auth/redeem. Unit tests cover the RFC 7636 S256 vector and the state match/mismatch cases.
  • Blank line no longer aborts login. read_auth_code returns Ok(None) on empty interactive input (auth.rs:323-326) and the reader loops, so a stray Enter re-prompts; only EOF cancels.
  • Non-tty stdin fails the preflight. is_browser_login_unavailable() checks CI and !stdin().is_terminal() (main.rs:369), so headless/piped auth login bails with an actionable message (pointing at --token-stdin / WAVEDASH_TOKEN, both of which are actually honored — auth.rs:122) instead of spawning a blocking reader.
  • Callback-failure recovery is immediate. After a failed callback redeem the loop spawns the paste reader right away (auth.rs:460+) rather than waiting on the 3s timer; a failed pasted code re-prompts immediately.
  • Redeem is bounded. redeem_auth_code carries a 10s timeout (auth.rs:406), so the browser connection can't hang indefinitely; on failure the 500 response + terminal fallback handle recovery.
  • Update-check suppression is correctly scoped to Update and Auth::Login (main.rs:415-421) so the auth prompts aren't interrupted; the notification is eprintln! (stderr) anyway.
  • --token-stdin is conflicts_with = "token" and trims/validates empty input (main.rs:381-388).

Non-blocking notes (all previously acknowledged by the author)

  • Leaked worker threads on the happy path. After a successful login the other worker can remain parked — the callback-server thread on listener.incoming(), or the stdin reader holding the process-wide stdin lock (auth.rs:318). Harmless because auth login exits immediately, but a latent hazard if login_with_browser is ever composed into a longer-lived interactive flow. A one-line comment noting the "process exits right after" assumption would help future readers. Fix this →
  • redeem blocks the select! loop for up to 10s, during which Ctrl-C is inert — bounded and intentional per the discussion.
  • SSH/manual-code users wait ~3s for the paste prompt (intentional so Paste code here if prompted > doesn't flash during a normal successful browser callback).
  • Minor: a pasted code exactly equal to a decoded callback path won't matter, but note handle_callback_request swallows Err(_)/Ok(None) from a malformed request silently (auth.rs:304-306) — intentional per the author (invalid attempts shouldn't poison the session).

Not verified

I did not compile or run the suite — the QA steps require Doppler-managed env vars (SITE_HOST/CONVEX_HTTP_URL/PLAYSITE_HOST) not available in this environment, and cargo/doppler aren't on the runner. Findings are from inspection; signatures for config::{get, create_http_client, check_api_response} were confirmed to match the call sites.
· branch kire5030/wvdsh-1727-cli-auth-codes

@cloud9c cloud9c requested a review from franzwarning July 7, 2026 18:21

Copy link
Copy Markdown
Member

ok just confirming that I liked the previous PRs changes too fwiw, with the json stuffz

cloud9c commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

gotcha -- then im thinking ill add them in a different PR? just didnt want u to have to review out of scope stuff

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9c79eaf. Configure here.

Comment thread src/main.rs
if is_browser_login_unavailable() {
anyhow::bail!(
"Browser login isn't available in this environment.\n\nCreate an API key at https://wavedash.com/dev-portal/keys. Then set WAVEDASH_TOKEN or pipe the key into wavedash auth login --token-stdin."
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded keys URL in error

Medium Severity

The browser login unavailable error message hardcodes the dev portal URL (https://wavedash.com/dev-portal/keys). This URL doesn't respect the configured SITE_HOST, which means users on staging or local builds might be directed to the wrong environment.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9c79eaf. Configure here.

Copy link
Copy Markdown
Member

yeah great call

@franzwarning franzwarning left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, don't merge til wavedash pr merged tho 🙂

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.

2 participants