Repository navigation
Revoke a token from Kotlin, unsigned, following no redirect - #270
Conversation
|
Behaviour review of the whole diff: no material findings. Checked: Added from the review: cases holding a revocation to none of a caller's strategy headers and no account filter on a scoped client. Not changing:
|
Codex Review SummaryThis 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. |
There was a problem hiding this comment.
🟡 Changes recommended
Rejected redirects report status 0 to request hooks instead of the received HTTP status.
1 open issue
What changed in this PR
Adds safe token revocation to the Kotlin SDK’s sign-out flow.
Changes:
- Sends revocations unsigned, with credentials in the form body.
- Rejects redirects without forwarding the token.
- Adds documentation and tests for revocation safety and operation hooks.
[!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 |
|---|---|
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/hey/RevocationTest.kt | Tests revocation behavior and safeguards. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/hey/Revocation.kt | Adds the token-revocation convenience. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/hey/Operation.kt | Adds internal redirect suppression. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/hey/HeyClient.kt | Rejects redirects when disabled. |
| kotlin/README.md | Documents token revocation. |
Review effort: Balanced
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: e0e1c957ad
ℹ️ 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".
…rect (red)
The tests run against the revocation a caller would write by hand: a
DELETE /oauth/tokens with the form body, through the client as any other
request. Three of the five fail:
- the path goes out as /oauth/tokens.json, which HEY does not route;
- a 401 is refreshed and the revocation sent again, with the credentials
the caller set out to end;
- a 307 is followed, and the token in the body goes to the Location's
host.
The two that pass (a 200 {} is no error; the hooks hear OAuth.RevokeToken)
pin what the fix must keep.
HeyClient.revokeToken(clientId, token) sends DELETE /oauth/tokens with
client_id and token in a form body under the bare path, as a sign-out
asks:
- never in the query, where logs would keep the token;
- unsigned: the client's own credentials may be the token being
revoked, so a 401 (invalid_client for another app's token) is the
answer, not a reason to refresh them and send the revocation again;
- following no redirect: a DELETE keeps its body on a 307, so a hop
would hand the token to the Location's host. Operation.noRedirects()
takes any 3xx as the answer, which maps to an api_error, with no hop.
unsigned() alone keeps following, as the storage reads it serves want.
HEY answers 200 {} for a token it no longer knows; that is no error.
The hooks hear it as OAuth.RevokeToken, a write.
/oauth routes sit outside the API model (drift-check-reverse skips
them), as the Go, Rust and TypeScript token exchanges do, so this is
hand-written beside them rather than generated.
Verified: RevocationTest 5/5 (three red on the hand-written revocation
in the previous commit), make kt-check kt-check-drift, Kotlin
conformance 219/219.
Not done: a conformance case. No other SDK revokes a token, the runners
read only top-level cases, and only TypeScript can mark one not
applicable, so a case would need a RevokeToken in four SDKs this change
does not touch.
The check now sits in transmit's redirect branch rather than on the line that picks the next hop: the caller's api_error carries the 3xx's status, and the line other changes to transmit touch stays as it was.
Review: the code already keeps a caller's own AuthStrategy off an unsigned request and leaves a scoped client's account filter off it; two cases now say so, so a later change that strips Authorization rather than skipping the signing, or scopes the path, fails here.
Review: refusing the hop by throwing from transmit sent the 3xx through the attempt loop's failure path, so the request hooks heard status 0, which means no response arrived, and the request id was lost. A request that follows no redirect now takes the 3xx as its answer, and the error mapping refuses it as for any status it does not accept: the caller still gets an api_error carrying the status, and the hooks hear 307. Shown red on the throw: the hooks heard [0], not [307]. revokeToken is a top-level extension, so the README's example now carries the import a caller needs.
b56bbab to
3d1fb32
Compare
|
Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in the thread). |
Outside review: under noRedirects a 3xx was still read for its body. A body lost mid-read became a network failure, which the retry loop answered by sending the revocation again (a DELETE is idempotent), and the hooks heard status 0 for both sends. A 3xx a request follows no redirect for now carries nothing the SDK reads, as a form's captured redirect does: its status and headers are kept, its body is not read, and the error mapping refuses it as an api_error with that status. Shown red first: a 307 whose body breaks mid-read surfaced a network error; now it is api_error 307, sent once, and the hooks hear [307].
|
Outside review round: under |
Brings in #268 and #269, which change HeyClient.kt's redirect and proof-refusal handling beside #270's revocation. Git merged them cleanly; make kt-check passes on the result. * origin/main: Refresh a redirected write's hop 401 without making the write again (#269) Resend a 401 that refuses only the DPoP proof, with no refresh (#268) Answer the rotation test's first two requests by token, not by arrival (#274) Wait for a reviewer before tagging the Go module and publishing Kotlin (#246) Bump the cargo group across 1 directory with 3 updates (#252) Find true on PATH in the release test, so make release runs on macOS (#266) Bump the gradle group across 1 directory with 4 updates (#255) Bump the cargo group across 1 directory with 4 updates (#253) Bump the typescript-dev group across 1 directory with 2 updates (#249) Bump golang.org/x/net from 0.60.0 to 0.61.0 in /conformance/runner/go in the go group across 1 directory (#251) Bump vite from 8.3.0 to 8.3.5 in /typescript (#272) Bump rustls-webpki and webpki-root-certs past the emSign advisories (#273)
Signing out of HEY ends the session behind a token with
DELETE /oauth/tokens. The Kotlin client had no way to send it safely: written by hand through the client, the revocation goes out under/oauth/tokens.json, refreshes and resends on a 401, and follows a redirect with the token in its body.What changes (Kotlin)
HeyClient.revokeToken(clientId, token):client_idandtokenin a form body under the bare path, never in the query, where logs would keep the token;invalid_clientfor another app's token) is the answer, not a reason to refresh them and send the revocation again;Operation.noRedirects()): a DELETE keeps its body on a 307/308, so a hop would hand the token to whatever host theLocationnames. Any 3xx is anapi_errorcarrying its status, with no hop.unsigned()alone still follows, as the storage reads it was made for want;{}for a token it no longer knows as no error;OAuth.RevokeToken, a write./oauthroutes sit outside the API model (drift-check-reverseskips them), and the Go, Rust and TypeScript token exchanges are hand-written for the same reason. So this is a hand-written convenience, not a generated operation.Red first
The first commit runs the tests against the revocation a caller would write by hand (
request(DELETE, "/oauth/tokens").form(...)). Three of the five fail:/oauth/tokens.json;The two that pass (200
{}is no error; the hooks' name) pin what the fix must keep.Verified
RevocationTest5/5,make kt-check kt-check-drift, Kotlin conformance 219/219. Nothing outsidekotlin/changes. The repo-widemake checkfailures main already has (cargo deny'swebpki-root-certsadvisory; locally, Swift 6.3 warnings-as-errors) are untouched by this. Merges cleanly with #268 and #269.Not done
RevokeTokenin four SDKs this change doesn't touch.Summary by cubic
Adds a
revokeToken(clientId, token)extension onHeyClientso signing out of HEY revokes the session token overDELETE /oauth/tokens, a route hand-written like the/oauthexchanges in the other SDKs.client_idandtokenin a form body under the bare path, never in the query where logs would keep the token.AuthStrategyheaders or account filter: a 401 is the answer, not a reason to refresh credentials and resend the revocation.Operation.noRedirects()): a DELETE keeps its body on a 307/308, so a hop would hand the token to the redirect target. Any 3xx is taken as the answer without its body read, so a body lost mid-read cannot turn into a retried revocation; it is heard by the hooks as its status and surfaced as aHeyException.Api.{}for a token it no longer knows as success and emits the hooks eventOAuth.RevokeTokenas a write; the README example shows the import a caller needs.Written for commit 2dd3b3a. Summary will update on new commits.