Uh oh!
There was an error while loading. Please reload this page.
feat(auth): Implement encrypted storage for OAuth account tokens - #853
Conversation
- Add AES-256-GCM encryption for OAuth access and refresh tokens - Create EncryptedPrismaAdapter wrapping Auth.js PrismaAdapter - Implement automatic encryption on token storage and decryption on usage - Support graceful handling of existing plaintext tokens with automatic migration - Update accountPermissionSyncer to decrypt tokens before API calls - Update tokenRefresh to encrypt refreshed tokens before storage This improves security by ensuring OAuth tokens are encrypted at rest in the database.
Caution Review failedThe pull request is closed. WalkthroughDecrypts stored OAuth tokens for runtime use and adds end-to-end encryption for OAuth tokens: shared encrypt/decrypt helpers, encrypted Prisma adapter and helpers, token encryption on sign-in/refresh, and decryption in backend permission sync flows. Changes
Sequence DiagramsequenceDiagram
participant OAuthProvider as OAuth Provider
participant NextAuth as NextAuth/AuthService
participant Adapter as EncryptedPrismaAdapter
participant DB as Prisma/Database
participant Backend as Permission Syncer
participant API as GitHub/GitLab API
OAuthProvider->>NextAuth: callback with tokens
NextAuth->>Adapter: linkAccount(account with tokens)
Adapter->>Adapter: encryptOAuthToken(access/refresh/id)
Adapter->>DB: store account (encrypted tokens)
DB-->>Backend: fetch account (encrypted tokens)
Backend->>Backend: decryptOAuthToken(account.access_token)
Backend->>API: call provider API with decrypted token
API-->>Backend: scopes/repos response
Backend->>Backend: validate permissions
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/shared/src/crypto.ts`:
- Around line 113-177: Add a fixed versioned encryption marker (e.g. "encv1:" or
magic bytes) to the output of encryptOAuthToken and make isOAuthTokenEncrypted
check for that marker rather than a length heuristic; update decryptOAuthToken
to expect that marker, strip it before base64-decoding, and treat any missing
marker as plaintext during migration but treat decoding/auth failures as a hard
failure (return null or throw depending on existing error policy) so we don't
silently accept bad ciphertext. Modify functions: isOAuthTokenEncrypted,
encryptOAuthToken, decryptOAuthToken, and keep deriveOAuthKey and constants but
add a constant for the marker/version to locate the logic easily.
In `@packages/web/src/lib/encryptedPrismaAdapter.ts`:
- Around line 9-46: encryptAccountData currently calls encryptOAuthToken on
possibly undefined values which returns null and causes Prisma update() to set
refresh_token=NULL; change encryptAccountData to only include
access_token/refresh_token/id_token keys when the incoming data has those keys
defined (i.e., check for data.access_token !== undefined, etc.) and only then
set the encrypted value (using encryptOAuthToken) so absent fields are left out
and do not overwrite existing DB values; locate and update the
encryptAccountData function (and any callers that expect its shape) to
conditionally add those properties rather than always spreading them as nulls.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/web/tools/decryptJWE.ts`:
- Around line 20-27: The decryptJWE function currently logs the result of
decode(...) even when decode returns null for invalid/expired tokens; update
decryptJWE to check the result of decode (the variable decoded) and if it is
null, log an explicit error message including context (e.g., "Failed to decode
token: invalid or expired") and exit non-zero or throw an error so failures
aren't masked; keep references to decode, decryptJWE, token, secret and salt
when adding the null-check and error handling.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts (1)
160-164:⚠️ Potential issue | 🟡 MinorPotential refresh loop when
expires_inis missing.If the OAuth provider doesn't return
expires_in,expiresAtis set to0. Since the refresh logic triggers whennow >= (expires_at - bufferTimeS)(line 44), a zero value would immediately qualify the token for refresh on the next request, potentially causing a refresh loop.Consider using a reasonable default expiration (e.g., 1 hour) or logging a warning:
🛡️ Proposed fix
const result = { accessToken: data.access_token, refreshToken: data.refresh_token ?? null, - expiresAt: data.expires_in ? Math.floor(Date.now() / 1000) + data.expires_in : 0,+ expiresAt: data.expires_in + ? Math.floor(Date.now() / 1000) + data.expires_in + : Math.floor(Date.now() / 1000) + 3600, // Default to 1 hour if not provided };
🤖 Fix all issues with AI agents
In `@packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts`:
- Around line 64-69: The update currently writes refresh_token:
encryptOAuthToken(refreshedTokens.refreshToken) which yields undefined when
refreshedTokens.refreshToken is null and causes Prisma to skip updating the
column; add validation around refreshedTokens.refreshToken before the prisma
update: if it is non-null, encrypt and include it in the update; if it is null,
explicitly log a warning (including provider/user identifiers) that the
refresh_token was not rotated so this behavior is visible; optionally after the
prisma update compare the stored encrypted refresh_token (or the update
response) against the previous value to assert it changed when a new token was
provided and surface an error if providers that should rotate tokens did not.
🧹 Nitpick comments (2)
packages/web/src/ee/features/permissionSyncing/tokenRefresh.ts (1)
36-81: Consider race condition with concurrent token refresh.Multiple simultaneous requests for the same user could trigger parallel refresh attempts for the same token. If the OAuth provider rotates refresh tokens (invalidating the old one), one refresh succeeds while others fail, causing spurious
RefreshTokenErrorentries.This is a known challenge with refresh token rotation. Consider:
- Adding a short-lived cache/lock per account to prevent concurrent refreshes
- Accepting this limitation and relying on the next request to succeed
Low priority if this edge case is acceptable for your use case.
packages/web/src/auth.ts (1)
220-224: Performance consideration: database query on every JWT callback.
refreshLinkedAccountTokensqueries the database on every authenticated request to check token expiration. While the actual OAuth refresh only occurs when tokens are near expiry, the database roundtrip adds latency to every request.Consider optimizing by:
- Caching the next expiration time in the JWT itself
- Only querying the database when approaching expiration
- Using a background job for token refresh instead of inline
This may be acceptable for current usage patterns, but worth noting for scalability.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This improves security by ensuring OAuth tokens are encrypted at rest in the database.
Additionally, this PR changes the refresh token path to source the provider tokens from the database rather than storing them in the JWT token.
Summary by CodeRabbit