Add opt-in refresh token support - #142
Closed
hpsin wants to merge 2 commits into
Closed
Conversation
GitHub OAuth apps can issue access tokens that expire after 8 hours along with a refresh token valid for 6 months. This adds support for that flow as an opt-in feature, so existing callers keep receiving non-expiring tokens with no code changes. Setting Flow.RequestRefreshToken requests the offline_access scope in both device flow and web application flow. Servers without support for expiring tokens ignore it and return a non-expiring token with no refresh token, so nothing assumes a refresh token was issued. api.AccessToken now records the expiration data the server returns, and api.Refresh exchanges a refresh token for a new token pair. TokenSource holds a token and refreshes it on demand, and NewHTTPClient wraps it in a client that attaches the token and, when a request is rejected, refreshes once and retries once, so a persistently rejected token cannot produce a refresh loop. Because refresh tokens are single-use, a refreshed token is adopted before the OnRefresh persistence callback runs and is retained even if that callback fails; losing it would strand the user. The callback runs without the lock held so it may re-enter the TokenSource. Closes cli#141 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds opt-in support for GitHub OAuth “expiring access tokens” and refresh tokens across the library, including scope injection (offline_access), capturing/deriving expiry metadata on tokens, a refresh exchange helper, and an HTTP transport/client that refreshes and retries once to avoid loops.
Changes:
- Add
RequestRefreshTokenopt-in plumbing for both device flow and web application flow (and lower-level helpers) by appendingoffline_accesswith dedupe. - Extend
api.AccessTokenwith expiry/refresh-expiry fields plusIsExpired()/CanRefresh(), and addapi.Refreshforgrant_type=refresh_token. - Introduce
oauth.TokenSource+oauth.Transport/oauth.NewHTTPClientto refresh on demand and retry a rejected request exactly once; update docs/examples/tests accordingly.
Show a summary per file
| File | Description |
|---|---|
| webapp/webapp_flow.go | Adds BrowserParams.RequestRefreshToken and injects offline_access scope when opted in. |
| webapp/offline_access_test.go | Tests webapp scope injection/dedupe and verifies caller scopes are not mutated. |
| webapp/examples_test.go | Updates webapp examples to demonstrate opt-in refresh-token usage. |
| transport.go | Adds new oauth.Transport with refresh-on-expiry and refresh-on-rejection (single retry). |
| transport_test.go | Adds transport tests for attach, refresh-once behavior, replayable bodies, and concurrency. |
| token_source.go | Adds oauth.TokenSource to hold/refresh tokens with OnRefresh persistence callback. |
| token_source_test.go | Adds tests for refresh semantics, concurrency, callback behavior, and error paths. |
| README.md | Adds “Expiring access tokens” adoption guide and links to updated examples. |
| oauth.go | Documents refresh-token/TokenSource usage at the package level and adds Flow.RequestRefreshToken. |
| oauth_webapp.go | Wires Flow.RequestRefreshToken into webapp flow params. |
| oauth_device.go | Wires Flow.RequestRefreshToken into device flow scope list via AppendOfflineAccess. |
| examples_test.go | Adds an end-to-end example showing opt-in + TokenSource + auto-refreshing HTTP client usage. |
| device/offline_access_test.go | Tests device flow WithOfflineAccess() scope injection/dedupe. |
| device/examples_test.go | Adds device example showing opt-in refresh token acquisition and refresh exchange. |
| device/device_flow.go | Adds device.WithOfflineAccess() helper to request the offline_access scope. |
| CHANGELOG.md | Adds changelog entry documenting the new opt-in refresh token support and APIs. |
| api/scopes.go | Introduces ScopeOfflineAccess and AppendOfflineAccess helper. |
| api/refresh.go | Adds api.Refresh refresh-token exchange and api.ErrRefreshTokenInvalid. |
| api/refresh_test.go | Tests refresh exchange success and all key error paths/parameter behavior. |
| api/access_token.go | Adds expiry parsing + derived timestamps and helper methods on AccessToken. |
| api/access_token_expiry_test.go | Tests expiry parsing, helpers, and AppendOfflineAccess immutability. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+141
to
+147
| if ts.OnRefresh != nil { | ||
| // Release the lock around the callback: it is caller-supplied code that may legitimately | ||
| // re-enter this TokenSource, and sync.Mutex is not reentrant. | ||
| ts.mu.Unlock() | ||
| err := ts.OnRefresh(newToken) | ||
| ts.mu.Lock() | ||
| if err != nil { |
Comment on lines
+34
to
+40
| base := t.Base | ||
| if base == nil { | ||
| base = http.DefaultTransport | ||
| } | ||
|
|
||
| ctx := req.Context() | ||
|
|
hpsin
commented
Aug 18, 2026
Comment on lines
+53
to
+54
| if t.RefreshTokenExpiresAt.IsZero() { | ||
| return true |
Author
There was a problem hiding this comment.
This seems inverted - if the RT_expires_at is 0 that seems like there's no RT and therefore it can't be refreshed.
hpsin
commented
Aug 18, 2026
| // WithOfflineAccess requests the "offline_access" scope, opting this authorization into receiving an | ||
| // expiring access token and a refresh token. Servers that do not support expiring tokens ignore it | ||
| // and issue a non-expiring token with no refresh token. | ||
| func WithOfflineAccess() AuthRequestEditorFn { |
Author
There was a problem hiding this comment.
Yelling at copilot about why this is different from webflow, makes no sense.
hpsin
marked this pull request as draft
August 18, 2026 20:59
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Member
|
Thanks for the PR, @hpsin! 🙏 I'm closing this as explained in #141 (comment). |
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.
Why
GitHub OAuth apps can be configured to issue access tokens that expire after 8 hours, paired with a refresh token valid for 6 months. Rotating tokens limits the blast radius of a leaked credential. This PR adds support for optionally requesting an expiring access token and refresh token via opt-in using the
offline_accessscope. It also handles graceful degredation back to a non-expiring token when talking to GHES, which won't support this til GHES 3.23.Note: There was already an
api.AccessToken.RefreshTokenfield, but nothing populated it in practice (no opt-in) and nothing could act on it.Closes #141
Approach
The feature is opt-in and additive. Existing callers get non-expiring tokens exactly as before with zero code changes.
Opting in. A single new field,
Flow.RequestRefreshToken, adds theoffline_accessscope to both device flow and web application flow. Per the docs, that scope opts an individual sign-in into expiring tokens even when the app is not globally configured for them. Callers using the lower-level packages use the matchingdevice.WithRefreshToken()andwebapp.WithRefreshToken()options.Capturing the response.
api.AccessTokennow recordsExpiresIn,ExpiresAt,RefreshTokenExpiresIn, andRefreshTokenExpiresAt, withIsExpired()andCanRefresh()helpers. Missing or unparseable expiry metadata never causes a usable token to be discarded, so a GHES instance that ignoresoffline_accessdegrades to exactly today's behavior.Redeeming it.
api.Refreshperforms thegrant_type=refresh_tokenexchange, omittingclient_secretfor device-flow tokens, and surfaces a rejected refresh token asapi.ErrRefreshTokenInvalid.Refresh once and retry.
oauth.TokenSourceholds a token and refreshes on demand;oauth.NewHTTPClientwraps it in a client that attaches the header, refreshes proactively on expiry, and on a rejected request refreshes once and retries once. The retry's response is returned unconditionally, so a persistently rejected token cannot produce a call/fail/refresh loop. The refresh request goes through a separate client rather than the wrapping transport, which is the other way a loop could form.Worth a careful look
A few decisions here are load-bearing, and all three came out of a review pass that caught them as bugs:
OnRefreshpersistence callback runs, and is retained even if that callback returns an error. Because refresh tokens are single-use, by the time the callback fires the old pair is already dead server-side and the new token is the only usable credential. Discarding it on a persistence failure would not restore the old token, it would just strand the user on a transient disk error. The error still reaches the caller.OnRefreshis invoked with the lock released. The README recommends sharing oneTokenSourcebehind anhttp.Client, which makes it very natural to write a callback that re-enters the source. Holding a non-reentrant mutex across it would deadlock the app. There is a regression test for this.RefreshTokenempty rather than carrying the old one forward. The old one was just consumed, so keeping it would persist a credential that reports itself refreshable but is guaranteed to fail.One judgment call: the transport treats a
403as a token problem only whenWWW-Authenticatementions expiry, so ordinary permission errors do not trigger a pointless refresh. A401is always treated as a token problem.@hpsin commentary on this judgement call - I do not trust GH APIs to reliably return a a www-authenticate with expiry, and we should probably treat all failures as worth a single refresh attempt with perhaps a 5 minute cooldown so that it's not spammed anyway.
Testing
gofmt,go vet, and the full suite under-race -count=2all pass. New tests cover expiry parsing (including the no-support and malformed cases), everyapi.Refresherror path, scope injection and dedupe in both flows,TokenSourceconcurrency and callback semantics, and the transport's refresh-once guarantee, including an explicit test that a server which always returns 401 produces exactly two requests and one refresh.Docs
The README gains an "Expiring access tokens" adoption guide aimed at someone updating an existing app: the one-line opt-in diff, the fields that now must be persisted, replacing manual
Authorizationheaders with aTokenSource, the single-use warning, an error-handling table, and a short checklist. Runnable examples were added for all three packages, plus godoc on the new symbols and a CHANGELOG entry.