feat(cli): revoke the session on logout instead of only deleting the file - #37
Merged
Conversation
…file `prk logout` deleted the credential file and stopped there. A refresh token stays valid at the authorization server until it expires or someone revokes it, so that left a working credential behind while reporting success -- on exactly the machine someone had decided to stop trusting. It now hands the token back first (RFC 7009), then deletes the file. The refresh token is the one revoked when there is one: section 2.1 has the server invalidate the access tokens issued from it, and it is the half worth ending, because an access token expires on its own within minutes while a refresh token is what keeps a stolen credential alive. Revocation is advisory and deletion is not. Revocation needs the network; deletion does not. If a failed request could fail the command, a machine with no connectivity could not be signed out at all -- a worse outcome than signing out locally and telling the server later, because the local credential is the part this machine controls. So the file goes whatever the server said, and a revocation that did not happen is a warning naming what is still live rather than a silent omission. `--no-revoke` skips the request outright, for a host where it would only stall. The endpoint is recorded at login so logout does not repeat discovery on the command most likely to run somewhere with a worse network. A credential written before that field existed -- which is every credential in existence as this lands -- falls back to discovering it from the stored issuer, rather than declining to revoke the sessions most likely to be signed out first. Under `--json`, `revoked` separates the three outcomes: true revoked, false attempted and not done, null not attempted. The warnings print even under `--json`, which otherwise leaves stderr byte-empty on success, for the same reason the unprotected-server warning does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| Some(endpoint) => endpoint, | ||
| None => return Revocation::Unsupported, | ||
| }, | ||
| Err(err) => { |
| .await | ||
| { | ||
| Ok(()) => Revocation::Revoked, | ||
| Err(err) => { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
prk logoutdeleted the credential file and stopped there. A refresh token stays valid at the authorization server until it expires or someone revokes it — so deleting the local copy made it unreachable from this machine and did nothing about the copy the server still honours.That meant
prk logoutreported success while leaving a working credential behind, on exactly the machine someone had decided to stop trusting. Nothing in the output hinted at it.What it does now
Hands the token back first (RFC 7009), then deletes the file. The wire request, captured against a mock endpoint:
The refresh token is the one revoked when there is one. RFC 7009 §2.1 has the server invalidate the access tokens issued from it, and it is the half worth ending: an access token expires on its own within minutes, while a refresh token is what keeps a stolen credential alive.
Revocation is advisory; deletion is not
Revocation needs the network. Deletion does not. If a failed request could fail the command, a machine with no connectivity could not be signed out at all — a worse outcome than signing out locally and telling the server later, because the local credential is the part this machine controls.
So the file goes whatever the server said, and a revocation that did not happen is a warning naming what is still live rather than a silent omission:
revoked: true, silent, exit 0Signed out., credential still deleted, exit 0--no-revokerevoked: null--no-revokeskips the request outright, for a host where it would only stall. Under--json,revokedseparates the three outcomes —truerevoked,falseattempted and not done,nullnot attempted. The warnings print even under--json, which otherwise leaves stderr byte-empty on success, for the same reason the unprotected-server warning does.Upgrade path
The endpoint is recorded at login, so logout does not repeat discovery on the command most likely to run somewhere with a worse network than the login had.
A credential written before that field existed — which is every credential in existence as this lands — falls back to discovering the endpoint from the stored issuer. Without that fallback the first logout after upgrading would never revoke, which is precisely the population most in need of it.
StoredSessiongains an optional field following the existingresourceprecedent, so older files load unchanged, and a session with nowhere to revoke writes no null field.The refresh path already reconstructs the session with
StoredSession { tokens, ..session }, so a token refresh cannot silently drop the endpoint.Testing
New integration tests drive real requests against a mock server: the form-post shape (token,
token_type_hint,client_id, and theapplication/x-www-form-urlencodedcontent type), RFC 7009 §2.2's rule that an unknown token is still a200and therefore not a failure, and a refusal surfacing the server's own error code. New unit tests cover the store round trip, a credential written before the field existed still loading, andskip_serializing_ifkeeping a null out of the file.Also verified by running the built binary against a live mock and against unreachable endpoints, confirming the credential is deleted in every failure path and that
--no-revokemakes no network call.mise run cigreen end to end:test:rust669,miri127,test:js1152,e2e101, plus the full lint/typecheck set.Not covered
prk logoutstill clears only the file backend, so a--storage keyringsession is not cleared. Pre-existing and unchanged here, but more visible now that logout does more.🤖 Generated with Claude Code