fix(web): Fix token refresh in back-compat case#841
Merged
brendan-kellam merged 12 commits intomainfrom Feb 7, 2026
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughAdds per-user refresh locks and refactors token refresh to a single-flight flow with zod validation and deprecated-env fallback; deprecates EE provider env vars in the env schema, updates docs and changelog, and removes commented EE vars from .env.development. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as Provider Config
participant Env as Deprecated Env Vars
participant OAuth as OAuth Provider
App->>Config: request provider config for provider
Config-->>App: config found? (yes/no)
alt config found
App->>OAuth: POST token endpoint (client_id/client_secret from config)
OAuth-->>App: access_token, refresh_token, expires_in
App-->>App: normalize & persist refreshed tokens
else config missing
App->>Env: getDeprecatedEnvCredentials(provider)
Env-->>App: client_id, client_secret, optional baseUrl
App->>OAuth: tryRefreshToken(env creds, refreshToken[, redirect_uri])
OAuth-->>App: access_token/refresh_token or error
alt refresh successful
App-->>App: normalize & persist refreshed tokens
else refresh failed
App-->>App: log error and record failure
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 similar comment
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 18: Update the changelog line that currently reads 'Fixed token refresh
error "Provider config not found or invalid for: x" when a sso is configured
using deprecated env vars.' to use the correct acronym casing and article:
replace "a sso" with "SSO" so the sentence reads e.g. '... when SSO is
configured using deprecated env vars.'; ensure only the phrase "a sso" is
changed to "SSO" in the affected line.
In `@packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts`:
- Around line 164-171: Validate the OAuth response body before returning: ensure
data.access_token exists and throw or return a handled error (so
encryptOAuthToken never receives undefined), and avoid setting expiresAt to 0 by
using a safe fallback instead of 0 (e.g., expiresAt: Math.floor(Date.now()/1000)
+ (data.expires_in ?? 300) or explicitly set expiresAt to null and document that
consumers (e.g., refreshLinkedAccountTokens) must handle null), so tokens
without expires_in don’t create an immediate refresh loop; update the function
that returns { accessToken, refreshToken, expiresAt } in tokenRefresh.ts
accordingly.
🧹 Nitpick comments (1)
packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts (1)
149-156: Consider adding a timeout to the fetch call.This is an external OAuth call with no timeout. If the provider endpoint is slow or unresponsive, this will hang indefinitely, potentially blocking the token refresh for all accounts in the
Promise.allat line 36.Proposed fix
const response = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', 'Accept': 'application/json', }, body: new URLSearchParams(bodyParams), + signal: AbortSignal.timeout(10_000), // 10 second timeout });
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts`:
- Around line 156-168: The URL construction for token refresh can produce double
slashes when baseUrl has a trailing slash; update the logic in tokenRefresh.ts
(the block that sets the url variable based on baseUrl and provider) to build
the endpoint using URL resolution instead of string concatenation—e.g., use new
URL('/login/oauth/access_token', baseUrl) for GitHub and new URL('/oauth/token',
baseUrl) for GitLab when baseUrl is present; keep the existing hardcoded
fallback URLs when baseUrl is absent and preserve the unsupported-provider error
path.
🧹 Nitpick comments (1)
packages/shared/src/index.server.ts (1)
33-33: Nit: missing trailing semicolon.Every other export statement in this file ends with a semicolon.
-export { env } from "./env.server.js" +export { env } from "./env.server.js";
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts`:
- Around line 89-93: The DB update unconditionally overwrites refresh_token with
encryptOAuthToken(refreshTokenResponse.refresh_token), which will wipe the
stored token when refresh_token is undefined; change the update in the token
refresh flow (where data: { access_token: encryptOAuthToken(...), refresh_token:
..., expires_at }) to only set/update the refresh_token field when
refreshTokenResponse.refresh_token is present — e.g., build the update payload
for the DB (or use a conditional property) so encryptOAuthToken is only called
and persisted if refreshTokenResponse.refresh_token !== undefined, keeping the
existing DB refresh token otherwise; reference encryptOAuthToken and
refreshTokenResponse to locate the change.
- Around line 241-243: The current error log in tokenRefresh.ts prints the full
OAuth response body (json) which may contain sensitive tokens; update the
failure branch (the block using result.success, logger.error, provider and json)
to avoid dumping the raw response and instead log only the Zod validation error
details (use result.error.message or result.error.format()) and contextual info
such as provider and request id if available; remove or redact
JSON.stringify(json, null, 2) from the logger.error call so no raw
tokens/secrets are logged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts`:
- Around line 78-81: The code sets expires_at = null when
refreshTokenResponse.expires_in is missing, which causes the account to be
excluded by the existing DB query (expires_at: { not: null }) and prevents
future refreshes; change the logic where refreshTokenResponse is processed (look
for refreshTokenResponse and the expires_at assignment in tokenRefresh.ts) to
compute expires_at as Math.floor(Date.now()/1000) +
(refreshTokenResponse.expires_in ?? 3600) (i.e., default to 3600s / 1 hour)
instead of null so the record remains eligible for refreshes; ensure any
save/update that writes expires_at uses this computed non-null value.
🧹 Nitpick comments (1)
packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts (1)
232-236: Consider redacting the error response body as well.
errorTextfrom a failed token refresh could also contain sensitive information (e.g., partial tokens in error descriptions from some providers). Consider truncating or sanitizing it before logging.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts`:
- Around line 127-138: The current logic logs "Provider config not found or
invalid" even when deprecated env credentials were found but
tryRefreshToken(provider, refreshToken, envCredentials) returned null; update
the branch handling providerConfigs.length === 0 to distinguish these cases:
call getDeprecatedEnvCredentials(provider) as before, and if envCredentials
exists but tryRefreshToken returns null, call logger.error with a message
indicating that token refresh using deprecated env credentials failed for that
provider (include provider and that deprecated creds were used), otherwise keep
the existing "Provider config not found or invalid" message when no
envCredentials exist; adjust logging in the block around providerConfigs,
getDeprecatedEnvCredentials, tryRefreshToken, and logger.error to reflect these
distinct outcomes.
🧹 Nitpick comments (2)
packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts (2)
229-236: Consider adding a timeout to thefetchcall.If the OAuth provider is slow or unresponsive, this
fetchwill hang indefinitely since there's noAbortSignalwith a timeout. Since this runs in a background refresh flow, a hung request would hold the per-user lock and block subsequent refresh attempts.Proposed fix
+ const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 30_000); + const response = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', 'Accept': 'application/json', }, body: new URLSearchParams(bodyParams), + signal: controller.signal, }); + + clearTimeout(timeout);
238-241: Error response body may contain sensitive data.
errorTextfrom the provider's error response could contain tokens, partial credentials, or internal details. Consider truncating or omitting it from logs, similar to how the success-path validation was tightened.
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
1 participant
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.
This PR fixes token refresh when using the deprecated (but still back-compat supported) env vars
AUTH_EE_GITHUB_CLIENT_IDandAUTH_EE_GITHUB_CLIENT_SECRET. It additionally adds better error handling for the refresh path.Summary by CodeRabbit
Bug Fixes
Documentation
Chores
Changelog