Repository navigation
fix(gatekeeper-github): refresh expiring tokens instead of breaking after 8 hours - #661
Merged
Merged
Conversation
… after 8 hours GitHub enables expiring user tokens by default for OAuth apps registered since 2026-08-14. The gatekeeper kept only the access token, so every connection on such an app failed with Bad credentials eight hours in. UserAccount now stores its grant through the kit's CredentialCoordinator, with the code exchange and refresh going through the kit's OAuthClient: an expiring grant is refreshed shortly before it expires, concurrent reads share one refresh, both rotated tokens are stored together, and GitHub's bad_refresh_token marks the grant dead and notifies the Workshop once. Non-expiring grants, including ones stored in the old layout, are served as before. A 401 is now adjudicated against the token the request presented: a token a refresh or reconnect replaced mid-request fails as retryable instead of marking the account expired, and a rejection of the current token refreshes past it where the grant can. Fixes #641
…egacy expiry latch Review follow-ups: - revoke() now moves the credential fence before its first await, so a refresh landing mid-disconnect is discarded and its tokens revoked instead of being stored and then deleted unrevoked. - Migrating a pre-refresh grant re-arms the expiry latch: that layout latched before delivering, so a failed delivery left a dead account showing as connected. - prepareReconnect no longer resets the latch by hand; every credential replacement re-arms it.
…t the fake's handler
|
LGTM! |
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…n in-flight token refresh A refresh invalidates the token a request already carries, so a configurator lookup or hasRepoAccess check overlapping one failed with GitHub's 401 although the replacement token works. Both now run through withAccountApi, which adjudicates the rejected token with the account and, for these replay-safe reads, reruns the call once with the replacement. A rejection the account confirms as grant death now also notifies the Workshop from these paths.
|
LGTM! |
Maximo-Guk
approved these changes
Oct 5, 2026
Member
|
🤖 ( take it or leave it! ) |
Maximo-Guk
reviewed
Oct 5, 2026
…a token refresh applyAction(postReview) creates the review, then reads its comments back before recording the action applied. A refresh that replaced the token one of those reads carried failed the apply after GitHub had accepted the review, leaving the action pending, so approving it again posted a second review. Those reads (and the review-comment sync they can fall back to) now go through #readApi, which reruns a read once with the replacement token. Mutations stay non-replayable.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
LGTM! |
Under the 2026-09-04 compatibility date (rpc_params_dup_stubs semantics) a caller keeps ownership of stubs it passes as params, so it must dispose them.
|
LGTM! |
ndisidore
added a commit
that referenced
this pull request
Oct 7, 2026
UserAccount hand-rolled what the kit's CredentialCoordinator exists for -- a refresh mutex, a transient-failure cooldown, and grantId, credentialId and deadGrantId fences -- and the plan deferred adopting it for reasons that stopped holding once gatekeeper-github adopted it (#661). The grant now lives in the coordinator, as GitHub's does: - A refresh redeems the single-use refresh token once however many callers wait, and stores the rotated pair before serving either. invalid_grant in a 4xx other than 429 (the kit's isInvalidGrant rule) is the grant's death, recorded with it and announced once through the credential-expiry latch. Every other failure -- network, 429, 5xx whatever its body, an Access redirect -- is transient, and #mintFailure repeats it to a burst of callers for a minute. - A refused token is judged against the token the request sent (reportTokenRejected). One the account has since replaced failed stale: a replayable read reruns once and any other call fails as retryable. A refusal of the live token is refreshed past before the grant is called dead. Permission 401s from merge and approve are still never reported. - Connects and reconnects record the connection generation they began under and commit through connect(grant, { ifGeneration }), so a disconnect or a faster reconnect wins and the loser's tokens are revoked. The observer probe's cached user id is fenced to the generation. - A refresh a disconnect overtook has the pair it minted revoked (discardMint): the disconnect revoked only the pair it found, which the refresh had already rotated out. One a reconnect overtook is dropped unrevoked, since GitLab does not document that revoking one refresh token spares the rest of the authorization. - The stub-era keys migrate into the coordinator's record on first read; a grant with no recorded scopes is still offered a reconnect. The connect flow uses the kit's handshake (putInitiation, advanceToOAuth, claimOAuth) and createPkce; /oauth answers 400 for a malformed state id and consumes the nonce when GitLab refuses. The refresh request no longer sends redirect_uri, which RFC 6749 does not define for it. Account reads, the viewer read and the configurators go through withAccountApi as replayable reads. storage-schema.md, the README's troubleshooting, and the plan's credential decisions, storage layout, OAuth helpers and deferred items describe the coordinator; the deferred "adopt kit credentials" item is removed. Tests: the account suite drives the real paths -- refresh and rotation, one redemption for concurrent callers, death only on invalid_grant and not in a 429 or 5xx body, a token replaced in flight replayed or failed as retryable, a refused live token refreshed past, reconnects overtaken or committed after a refresh, a refresh overtaken by a disconnect (revoked) or a reconnect (not), and the stub-era migration. The observer probe does not admit a user reconnected mid-read on the previous user's membership, and the OAuth relay refuses a malformed state id and consumes a refused attempt.
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
3 participants
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.
Fixes #641. OAuth apps registered since August 2026 get GitHub access tokens that expire after 8 hours, and the gatekeeper threw away the refresh token, so every connection on those apps broke 8 hours in. The account now keeps its grant in the kit's
CredentialCoordinatorand refreshes the token a minute before it expires, one refresh at a time. When GitHub rejects the refresh token itself, the Workshop hears about it once and the account shows Reconnect. A 401 on a token that a refresh replaced mid-request no longer marks the account expired: configurator lookups, observer checks and a review apply's follow-up reads retry once with the new token, and every other call fails with "please retry". Tokens that don't expire, including ones stored by the old code, work as before. New workerd tests run against a fake GitHub and cover refresh, rotation, a dead refresh token, a refresh racing a disconnect or an in-flight read, and the migration; one canceled-request warning in the test run is still unexplained.