Skip to content

verify auth status against the server - #65

Open
The64thRealm wants to merge 5 commits into
mainfrom
auth-status-uses-endpoint
Open

verify auth status against the server#65
The64thRealm wants to merge 5 commits into
mainfrom
auth-status-uses-endpoint

Conversation

@The64thRealm

Copy link
Copy Markdown
Contributor

auth status used to print whatever email login cached at the time, which is wrong the moment the key is revoked or the account goes away, and was blank for keys supplied by hand or by WAVEDASH_TOKEN

auth status used to print whatever email login cached at the time,
which is wrong the moment the key is revoked or the account goes away,
and was blank for keys supplied by hand or by WAVEDASH_TOKEN.

GET /api/me returns the owner of the key, so status calls that and
prints the username and email it gets back. A 401 is reported as a
rejected key with exit 1, pointing at login or the env var depending
on where the key came from. Any other failure is reported as unable
to verify, still showing the masked key.

Nothing reads the cached email any more, so the credentials file
stops carrying it. Old files with an email field still load.
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 4m 40s —— View job


Code review

  • Gather context (diff, AuthInfo/LoginResult/Credentials/store_credentials consumers, config::check_api_response)
  • Review src/auth.rs and src/main.rs
  • Post findings as inline comments

Reviewed commit fc7001f. The direction is right — checking the key against /api/me instead of echoing a cached email fixes both the stale-email and the blank-email-for-manual-keys problems, and the Verification::{Valid,Rejected} split keeps "the server said no" distinct from "I couldn't ask." Dropping email from Credentials is backward-compatible (serde ignores unknown keys), src/init.rs:253 is the only other get_auth_info caller and is unaffected, and the new axum test server covers the three interesting responses. Note: cargo wasn't runnable in this environment, so type-checking is by inspection only.

4 findings, posted inline:

# Location Issue
1 src/auth.rs:208 Non-401 failures go through the game-oriented config::check_api_response, so a 404 makes auth status say "Game not found… your wavedash.toml may be incorrect" and a 403 says "check that you're logged in with the right account (wavedash auth status)" — advice that loops back on itself. Only 401 reaches Verification::Rejected, even though its message claims to also cover an inactive account.
2 src/auth.rs:45 Identity.email/username are required String. Every representation of email this PR deletes was Option<String>; if /api/me ever returns a null/absent email, a perfectly valid key reports "could not verify it with the server: error decoding response body".
3 src/auth.rs:200 No .timeout(…) on the request and no default timeout on the client, so auth status hangs indefinitely against an unresponsive host. The sibling redeem_auth_code sets AUTH_CODE_REDEEM_TIMEOUT for exactly this.
4 src/main.rs:55 Exit codes are now inconsistent: a rejected key exits 1, but having no credentials at all still exits 0, so wavedash auth status || wavedash auth login silently skips login on a fresh runner.
· branch auth-status-uses-endpoint

devin-ai-integration[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 3m 28s —— View job


Code review

  • Gather context (diff vs origin/main, both commits, Identity/Verification/check_api_response, remaining email consumers)
  • Review src/auth.rs and src/main.rs
  • Post findings as inline comments

Reviewed at 9b5c3b1, which adds the exit-1-when-unauthenticated change on top of fc7001f. That resolves the exit-code inconsistency raised on the previous pass — auth status is now a usable || auth login guard. The other three findings still stand against the current head.

3 findings, posted inline:

# Location Issue
1 src/auth.rs:200 No .timeout(…) on the /api/me request and no default on the client, so auth status hangs indefinitely against a blackholed api_host instead of reporting "could not verify". redeem_auth_code in this same file guards against exactly this.
2 src/auth.rs:208 Non-401 failures go through the game-oriented config::check_api_response: a 404 becomes "Game not found… your wavedash.toml may be incorrect" and a 403 becomes "check that you're logged in (wavedash auth status)" — self-referential advice from auth status itself. Only 401 reaches Verification::Rejected, though its message claims to cover inactive accounts too.
3 src/auth.rs:44 Identity.email is a required String while every email representation this PR deletes was Option<String>; a 200 with "email": null makes decoding fail, so a valid key reports as unverifiable and exits 1.

Checked and deliberately not flagged: dropping email from Credentials is backward compatible (no deny_unknown_fields, so serde ignores the stale key); src/init.rs:253 is the only other get_auth_info consumer and reads just source/api_key; axum/tokio are non-dev dependencies so the new test server compiles; the three new tests cover valid / rejected / undecodable responses.

Note: cargo check isn't runnable in this environment (permission denied), so compilation and types were verified by reading only.

· branch auth-status-uses-endpoint ·

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 4m 7s —— View job


Code review

  • Gather context (diff vs origin/main, all three commits, Identity/Verification/check_api_response, --json path)
  • Review src/auth.rs and src/main.rs
  • Post findings as inline comments

Reviewed at ea78dfc, which adds --json on top of the previous two commits. The AuthStatus struct with camelCase renames is a clean way to do it, and the JSON stream stays uncontaminated (first-run splash is TTY-gated, update notices go to stderr). One new finding from this commit; the three from the earlier passes still stand unaddressed against the current head.

4 findings, posted inline:

# Location Issue
1 src/main.rs:50 The JSON field is named apiKey but holds mask_token(&api_key), so jq -r .apiKey hands a script wd_a…f3x-style filler that will be sent as a bearer token and rejected. A name like maskedApiKey makes the value self-describing.
2 src/auth.rs:201 No .timeout(…) on the /api/me request and no default on the client, so auth status hangs indefinitely against a blackholed api_host instead of reporting "could not verify". redeem_auth_code in this same file guards against exactly this.
3 src/auth.rs:209 Non-401 failures route through the game-oriented config::check_api_response: a 404 becomes "Game not found… your wavedash.toml may be incorrect" and a 403 becomes "check that you're logged in (wavedash auth status)" — self-referential advice from auth status itself.
4 src/auth.rs:48 Identity.email is a required String while every email representation this PR deletes was Option<String>; a 200 with "email": null makes decoding fail, so a valid key reports as unverifiable and exits 1.

Checked and deliberately not flagged: the partial move of auth_info.api_key alongside the later auth_info.source read is sound (source is borrowed in the match, moved only in the JSON arm); legacy credentials.json files still carrying email deserialize fine (no deny_unknown_fields); the AuthSource serde renames line up with the tests; src/init.rs:253 is the only other get_auth_info consumer and reads just source/api_key.

Note: cargo check isn't runnable in this environment (sandbox denied), so compilation and types were verified by reading only.
· branch auth-status-uses-endpoint

devin-ai-integration[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

claude[bot]

This comment was marked as resolved.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 6m 11s —— View job


Code review

  • Gather context (diff vs origin/main, all four commits, CI status)
  • Review src/auth.rs and src/main.rs
  • Post findings as inline comments

Reviewed at 9271238, which drops the masked key from the --json payload — that resolves the apiKey-holds-a-mask finding from the previous pass, and dropping the field entirely is better than renaming it. CI ("test and lint") is green on this head.

Verified the tricky parts hold: removing email from Credentials is backward compatible (no deny_unknown_fields, so existing credentials.json files still deserialize), the partial move of auth_info.api_key alongside the later auth_info.source read is sound, nothing else in the tree read AuthInfo.email / LoginResult.email, and the update notice goes to stderr so it can't contaminate --json stdout.

4 findings, posted inline:

# Location Issue
1 src/auth.rs:206 Every 401 is attributed to the API key. A Cloudflare Access 401 (staging build without the option_env! CF creds) is reported as "it may have been revoked" and sends the user to re-login for a key that's fine.
2 src/auth.rs:209 Non-401 responses still route through the game-oriented config::check_api_response, so a 403 from the identity endpoint yields "Access denied … check that you're logged in (wavedash auth status)" — self-referential — and a 404 yields "Game not found … your wavedash.toml may be incorrect".
3 src/main.rs:84 --json only emits JSON on success; not-authenticated, rejected, and could-not-verify all bail with prose on stderr, so a script can't distinguish "revoked" from "server unreachable" — the two cases --json is most useful for.
4 src/main.rs:65 mask_token byte-slices (&token[..6]), so a token with a multibyte char straddling byte 6 panics with "byte index 6 is not a char boundary" instead of reporting the key as rejected. Pre-existing code, but handle_auth_status is now its only, unconditional call site.

Still open from earlier passes (unchanged at this head, not re-posted inline to avoid duplicate threads):

  • src/auth.rs:201 — no .timeout(…) on the /api/me request and no default on the client, so auth status hangs indefinitely against a blackholed api_host. redeem_auth_code in this same file guards against exactly this with AUTH_CODE_REDEEM_TIMEOUT. Fix this →
  • src/auth.rs:39Identity.email is a required String while every email representation this PR deletes was Option<String>; a 200 with "email": null makes decoding fail, so a valid key reports as unverifiable and exits 1. Fix this →

Note: cargo check isn't runnable in this environment (sandbox denied), so compilation and types were verified by reading only; CI covers the build.
· branch auth-status-uses-endpoint

Comment thread src/auth.rs
.header("Authorization", format!("Bearer {}", api_key))
.send()
.await?;
if response.status() == reqwest::StatusCode::UNAUTHORIZED {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Every 401 is attributed to the API key, including 401s that have nothing to do with it. Staging builds bake in the CF Access creds through option_env! (config.rs:80-81), so a staging binary built without them gets a Cloudflare Access 401 on every request — and auth status then reports "The API key ... was rejected by the server. It may have been revoked, or the account may no longer be active" and tells the user to log in again, which cannot fix a CF Access rejection. Consider only treating a 401 as Rejected when the body is the API's own JSON error (e.g. code == "unauthorized"), and falling through to check_api_response otherwise.

claude[bot]

This comment was marked as resolved.

Comment thread src/main.rs Outdated
}
Ok(())
}
Ok(Verification::Rejected) => anyhow::bail!(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

--json only produces JSON on the happy path. The three interesting verdicts this PR introduces — not authenticated (line 63), key rejected (here), and could-not-verify (line 87) — all bail! with prose on stderr, so a script doing wavedash auth status --json | jq -r .email gets empty stdout and has to parse English from stderr to tell "revoked key" apart from "server unreachable". Emitting something like {"authenticated": false, "reason": "rejected"} on stdout (still exit 1) in json mode would make the machine-readable mode usable for the case it's most needed.

claude[bot]

This comment was marked as resolved.

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 5m 23s —— View job


Code review

  • Gather context (diff vs origin/main, all five commits, CI status)
  • Review src/auth.rs and src/main.rs
  • Post findings as inline comments

Reviewed at aa263a9, which adds JSON failure verdicts on stdout — that resolves the "--json only emits JSON on the happy path" finding from the previous pass, and the {verdict:#} pretty-print matches the repo's to_string_pretty convention. CI ("CI", "Release") is green on this head.

Verified the tricky parts hold: dropping email from Credentials is backward compatible (no deny_unknown_fields, so existing credentials.json files still deserialize), the partial move of auth_info.api_key alongside the later auth_info.source read is legal, and the update notice goes to stderr so it can't corrupt --json stdout.

4 findings, posted inline:

# Location Issue
1 src/auth.rs:204 No .timeout(…) on the /api/me request and no default on the client, so auth status hangs indefinitely against a blackholed api_host. redeem_auth_code in this same file guards against exactly this with AUTH_CODE_REDEEM_TIMEOUT.
2 src/auth.rs:209 Only 401 maps to Rejected; everything else routes through the game-oriented check_api_response, so a 403 (revoked key, CF Access) reports reason: "unverified" with "you don't have permission to access this game … run wavedash auth status", and a 404 blames wavedash.toml.
3 src/main.rs:97 Both failure verdicts embed \nAPI Key: {masked} in message, putting the masked key back into --json output that commit 9271238 deliberately removed it from.
4 src/main.rs:82 The deleted auth_status_json_has_only_source_username_and_email test wasn't replaced, so nothing pins the --json success key set or the lowercase AuthSource serialization.

Still open from earlier passes (unchanged at this head, not re-posted inline to avoid duplicate threads):

  • src/auth.rs:39Identity.email is a required String while every email representation this PR deletes was Option<String>; a 200 with "email": null makes decoding fail, so a valid key reports as unverifiable and exits 1. Fix this →
  • src/main.rs:65mask_token byte-slices (&token[..6]), so a token with a multibyte char straddling byte 6 panics with "byte index 6 is not a char boundary" instead of reporting the key as rejected. Pre-existing, but handle_auth_status is now its only, unconditional call site. Fix this →

Note: cargo check / cargo test weren't runnable in this environment (command required approval), so the new axum-based tests are unverified by execution here; CI covers the build.

· branch auth-status-uses-endpoint ·

Comment thread src/auth.rs
Comment thread src/auth.rs
Comment thread src/main.rs
Comment thread src/main.rs
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