Repository navigation
Conversation
`fizzy signup` mints a write token ("Fizzy CLI") for the account it
sets up, and `fizzy auth logout` only forgot it: the token stayed valid
at Fizzy until someone revoked it by hand. Logout now revokes that token
at Fizzy's revocation endpoint (RFC 7009, which takes a personal access
token on possession) before removing it, best effort: a failure is
reported on stderr and as revoke_error, and the credential is removed
either way. The result carries `revoked`.
A token given to `fizzy auth login` is only removed, as before. It may be
in use elsewhere, so it isn't the CLI's to revoke. The profile records
which token signup created by its SHA-256, never the token, and any other
save drops the mark. A mark left beside a different token matches
nothing, so a pasted token is never revoked.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
fizzy-cli/internal/commands/auth.go
Lines 160 to 161 in f13807e
Report revocation failures before returning cleanup errors
If the remote revocation fails and a later local cleanup operation also fails, this early return skips reportMintedTokenRevocations, so the user never receives the warning that the write token remains valid. This is especially problematic when credential/profile deletion succeeded but saving the global config failed: the mark and stored token are already gone, a retry cannot attempt revocation again, and the server-side token is left active without the promised warning; the authLogoutAll cleanup-error path has the same issue.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Logout can report complete revocation after partial failure and omit warnings that remote tokens remain valid when local cleanup also fails.
2 open issues
What changed in this PR
This PR adds best-effort revocation of signup-created tokens to the CLI’s logout flow, while preserving local-only removal for user-supplied tokens.
Changes:
- Track signup-created tokens using SHA-256 profile metadata and clear the mark on other credential saves.
- Revoke matching tokens before logout cleanup, with timeout protection, warnings, and structured results.
- Add tests for successful revocation, failures, token replacement, and multi-profile logout.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/commands/signup.go |
Marks signup-created credentials as CLI-minted. |
internal/commands/root.go |
Allows profile saves to update extra metadata. |
internal/commands/profile_state.go |
Records or clears token ownership metadata during saves. |
internal/commands/minted_token.go |
Implements digest matching and bounded revocation requests. |
internal/commands/minted_token_test.go |
Tests token ownership and logout revocation behavior. |
internal/commands/auth.go |
Integrates revocation into logout, reporting, and help text. |
Review effort: Balanced
The revocation warning now prints as each revocation fails, before any cleanup step can return early. revoked is true only when every due revocation went through, and revoke_error names each that didn't. A credential store that can't be read counts as a failed revocation, not an absent token. All of one logout's revocations share a 20 s deadline, and tests cover the no-redirect client and restore stderr on failure.
|
Fixed — the Codex review finding "Report revocation failures before cleanup errors" (auth.go L160): since 7c5bc47 the warning prints as each revocation fails, before any cleanup error can return. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…he command context in logout --all
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c35e418fd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review threads: 13 resolved (12 fixed, 1 declined with the reasoning in the thread). Fixed from a review body:
Declined:
Not reported on the head: |
Summary
fizzy signupcreates a write token ("Fizzy CLI") for the account it sets up. Until nowfizzy auth logoutonly forgot it, so the token stayed valid at Fizzy until someone revoked it by hand. Logout now revokes that token first, then removes it as before.fizzy auth loginis still only removed. It may also live in a password manager, CI or another tool, so it isn't the CLI's to revoke. The help text says so.saveSignupConfigrecords the token's SHA-256 (never the token) under the profile'sminted_token_sha256. Any other save throughsaveProfileCredentialState, such asauth login, drops that mark. Logout revokes only a stored token whose digest matches. A stale mark next to a different token matches nothing, so a pasted token is never revoked, whichever path stored it.POST <profile base URL>/oauth/revocation(RFC 7009), with a 10 s timeout and no redirects followed. Fizzy accepts a personal access token there on possession and answers 200. Any other answer, or no answer, still removes the credential, prints a warning on stderr pointing at Fizzy's access tokens, and setsrevoke_error. The result now carriesrevoked: true|false.--alldoes the same for every profile.Compatibility
/oauth/revocation(404) or with an outage means a local logout plus the warning.Tests
internal/commands/minted_token_test.go:revoked: falsewith the error and no token in it;--allrevokes both signup tokens and not the pasted one.make lint,go vet ./...,gofmt -l .andgo test -race ./internal/commands/pass, as doesgo test ./....Summary by cubic
fizzy auth logoutnow revokes the write token thatfizzy signupcreated before removing it, so the token no longer stays valid at Fizzy. Tokens handed tofizzy auth loginare still only forgotten, never revoked, since they may be in use elsewhere.revokedistrueonly when every due revocation, including each in--all, went through; any failure reportsrevoked: falsewithrevoke_errornaming the profile. All revocations in one logout share a 20 s deadline. When the credential store can't be read, the fallback config-file token is matched and revoked if it belongs to signup; otherwise an unreadable store counts as a failed revocation, as does a signup token the profile no longer stores, which may still be live.Written for commit e64b278. Summary will update on new commits.