Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces cookie-based OAuth callback deduplication with a Redis-backed per-code lock and 60s result cache. Callback routes short-circuit on cached results, acquire per-code locks to prevent concurrent processing, persist success results, clear Redis entries on errors, and remove several cookie-based helpers/constants. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Provider as Google/Outlook
participant Callback as Callback Route
participant Redis
participant Accounts
rect rgb(240,250,240)
Note over Callback,Redis: Per-code lock + 60s result cache
end
Client->>Provider: OAuth flow -> redirect with code
Client->>Callback: GET /callback?code=CODE
Callback->>Redis: getOAuthCodeResult(CODE)
alt Cache Hit (success)
Redis-->>Callback: {status:"success", params}
Callback-->>Client: Redirect /accounts?success=1¶ms...
else Cache Miss
Redis-->>Callback: null
Callback->>Redis: acquireOAuthCodeLock(CODE)
alt Lock Acquired
Redis-->>Callback: true
Callback->>Provider: Exchange CODE for token
Provider-->>Callback: token + user info
Callback->>Callback: create or merge account
Callback->>Redis: setOAuthCodeResult(CODE, params)
Redis-->>Callback: ok
Callback-->>Client: Redirect /accounts?success=1¶ms...
else Lock Not Acquired
Redis-->>Callback: false
Callback-->>Client: Redirect /accounts (short-circuit)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/web/utils/redis/oauth-code.ts (2)
4-10: Hashing choice and CodeQL warning look acceptable; consider documenting intentUsing a single
sha256and truncating to 16 hex chars for the Redis key is fine here since the OAuth code is short‑lived, only used to derive a cache key, and expires after 60s. The CodeQL “password hash with insufficient computational effort” warning appears to be a false positive in this context. You might add a brief comment that this hash is for keying/deduplication only (not password storage) to avoid future confusion.
17-24: Lock/result semantics are sound; small edge case around non-success flowsThe pattern of:
SET key "processing" NX EX 60as a lock, and- overwriting the same key with the success payload (also EX 60) for dedupe
is a good fit for preventing duplicate processing and serving cached success responses.
One minor edge case: flows that return early without calling
setOAuthCodeResult(e.g., if upstream callers exit on a redirect‑type linking result) will leave the key at"processing"until TTL expiry, causing subsequent callbacks for that code to hit the “being processed” path rather than a cached result. If you expect duplicate callbacks in those paths and want consistent behavior, consider either clearing the key or also caching a “non-success” result there.Also applies to: 26-40, 42-55
apps/web/app/api/google/linking/callback/route.ts (2)
39-64: Redis-backed dedupe logic looks correct; note behavior of second callbacks during in-flight processingThe sequence:
- Try
getOAuthCodeResult(code)and, on hit, replay the original success params.- Otherwise attempt
acquireOAuthCodeLock(code), and if it fails, redirect to/accountsand clear the state cookie.is a solid way to avoid duplicate processing and should prevent spurious error toasts from second callbacks using the same code. One UX nuance: in the lock-fail path the redirect carries no
successorerrorquery, so the user just ends up on/accountswith a neutral state. If you want the same success toast on concurrent callbacks, you might consider a short polling window or another mechanism to reuse the cached result instead of immediately bailing.
172-179: Success handling duplicated; consider a small helper for consistencyBoth the “new account created” and “merge” branches:
- call
setOAuthCodeResult(code, { success: ... }),- build
/accounts?success=..., and- delete
GOOGLE_LINKING_STATE_COOKIE_NAMEbefore redirecting.This pattern is also mirrored in the Outlook callback. A tiny helper (e.g. something like
redirectWithOAuthSuccess({ request, code, success, stateCookieName })) would reduce duplication and make it harder for the flows to drift apart in future changes.Also applies to: 210-217
apps/web/app/api/outlook/linking/callback/route.ts (2)
42-67: Outlook callback dedupe mirrors Google logic correctlyThe cache-first, then lock acquisition flow matches the Google handler and should similarly avoid duplicate processing and spurious errors for reused codes. As with the Google route, the lock-fail path redirects to
/accountswithout a success/error query, which keeps things neutral but doesn’t replay the original success toast; if you want identical UX for concurrent callbacks, you could consider a small polling or reuse mechanism instead of a bare redirect.
243-250: Success redirect logic is duplicated and could be sharedThe “create” and “merge” branches both:
setOAuthCodeResult(code, { success: ... }),- build
/accounts?success=..., and- delete
OUTLOOK_LINKING_STATE_COOKIE_NAMEbefore redirecting.Given this is almost identical to the Google route, a shared helper for “write success to Redis and build the standard
/accountsredirect” would reduce repeated code and keep provider behavior aligned.Also applies to: 279-286
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/api/google/linking/callback/route.ts(4 hunks)apps/web/app/api/outlook/linking/callback/route.ts(4 hunks)apps/web/utils/gmail/constants.ts(0 hunks)apps/web/utils/oauth/callback-helpers.ts(0 hunks)apps/web/utils/oauth/error-handler.ts(0 hunks)apps/web/utils/oauth/state.ts(0 hunks)apps/web/utils/outlook/constants.ts(0 hunks)apps/web/utils/redis/oauth-code.ts(1 hunks)
💤 Files with no reviewable changes (5)
- apps/web/utils/gmail/constants.ts
- apps/web/utils/outlook/constants.ts
- apps/web/utils/oauth/state.ts
- apps/web/utils/oauth/error-handler.ts
- apps/web/utils/oauth/callback-helpers.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/api/google/linking/callback/route.ts (3)
apps/web/utils/redis/oauth-code.ts (4)
getOAuthCodeResult(26-40)acquireOAuthCodeLock(17-24)setOAuthCodeResult(42-52)clearOAuthCode(54-56)apps/web/utils/gmail/constants.ts (1)
GOOGLE_LINKING_STATE_COOKIE_NAME(16-16)apps/web/utils/oauth/error-handler.ts (1)
handleOAuthCallbackError(11-25)
apps/web/app/api/outlook/linking/callback/route.ts (3)
apps/web/utils/redis/oauth-code.ts (4)
getOAuthCodeResult(26-40)acquireOAuthCodeLock(17-24)setOAuthCodeResult(42-52)clearOAuthCode(54-56)apps/web/utils/outlook/constants.ts (1)
OUTLOOK_LINKING_STATE_COOKIE_NAME(1-1)apps/web/utils/oauth/error-handler.ts (1)
handleOAuthCallbackError(11-25)
🪛 GitHub Check: CodeQL
apps/web/utils/redis/oauth-code.ts
[failure] 5-5: Use of password hash with insufficient computational effort
Password from a call to validateOAuthCallback is hashed insecurely.
Password from a call to validateOAuthCallback is hashed insecurely.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/web/app/api/google/linking/callback/route.ts (1)
219-227: Good: lock cleanup on error pathClearing the OAuth code key in the catch block before delegating to
handleOAuthCallbackErrorensures you don’t leave the “processing” sentinel stuck in Redis after errors, which would otherwise block future attempts for up to the TTL.apps/web/app/api/outlook/linking/callback/route.ts (1)
288-296: Good: clearing OAuth code on error prevents stale locksCalling
clearOAuthCode(code)in the catch block before delegating tohandleOAuthCallbackErroravoids leaving a “processing” value hanging in Redis after failures, which keeps future attempts from being blocked by a stale lock.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/redis/oauth-code.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
apps/web/utils/redis/oauth-code.ts
[failure] 6-6: Use of password hash with insufficient computational effort
Password from a call to validateOAuthCallback is hashed insecurely.
Password from a call to validateOAuthCallback is hashed insecurely.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/web/utils/redis/oauth-code.ts (2)
4-7: Static analysis false positive - implementation is appropriate.The CodeQL warning about "insecure password hashing" is a false positive. This function generates cache keys for short-lived OAuth authorization codes, not password hashes. The comment on line 4 correctly clarifies this distinction.
The truncation to 16 hex characters (64 bits) provides sufficient collision resistance for the use case: OAuth codes that live only 60 seconds and are randomly generated.
18-25: LGTM! Solid distributed lock and cache implementation.The lock acquisition using Redis SET with the NX flag provides atomic semantics, preventing race conditions. The consistent 60-second TTL across lock and result storage ensures proper cleanup. The clear/delete operation completes the lifecycle management.
Also applies to: 43-57
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/web/app/api/google/linking/callback/route.ts (1)
56-65: Consider adding user feedback when lock acquisition fails.When another request is already processing the OAuth code, the user is redirected to
/accountswithout any query parameters. While this prevents duplicate processing, the user receives no feedback about what happened or whether the linking succeeded.Consider adding a query parameter (e.g.,
?info=processingor?success=pending) so the UI can inform the user that their request is being processed.const redirectUrl = new URL("/accounts", request.nextUrl.origin); + redirectUrl.searchParams.set("info", "already_processing"); const response = NextResponse.redirect(redirectUrl);apps/web/app/api/outlook/linking/callback/route.ts (3)
43-68: Redis dedupe and lock flow is solid; consider UX for lock contention pathThe
getOAuthCodeResult+acquireOAuthCodeLocksequence correctly ensures:
- Finished callbacks short‑circuit using cached params, and
- Only one request per code actually does the heavy work.
However, when the lock isn’t acquired, the handler immediately redirects to
/accountswithout anysuccessorerrorquery params. Depending on how the/accountspage decides to show the “connect failed” toast, this “lock contention” path might still end up surfacing an error even though a concurrent request is legitimately processing the link.If the frontend currently treats “no success param” as a failure, consider:
- Either making the losing request wait briefly and re-check
getOAuthCodeResultbefore redirecting, or- Agreeing on a neutral flag (or success reuse) that prevents a spurious error toast in this branch.
This is more of a UX/flow refinement than a correctness bug, but worth double‑checking against the
/accountslogic.
212-281: Account creation + duplicate handling look correct; minor clarity improvements possibleThe new
microsoftProviderAccountIdconst and the create block wrapped intry/catchwithisDuplicateErrorgive you robust handling for concurrent account creation:
- You correctly reuse
profile.id || providerEmailas the providerAccountId and check for a concurrent create by re‑querying and comparinguserIdtotargetUserId.- On true concurrency, you log and continue, and on any other duplicate you rethrow, which is the right behavior.
Two small refinements you might consider:
Narrow the duplicate check explicitly to the provider/providerAccountId index for readability
Even though the follow-up
findUniqueguard already makes this effectively safe, making intent explicit would help future readers:
} catch (createError: unknown) {if (isDuplicateError(createError)) {
} catch (createError: unknown) {if (isDuplicateError(createError, "provider_providerAccountId")) {
Avoid duplicating the providerAccountId expression
The expression
profile.id || providerEmailis used both in the earlierfindUniqueand here viamicrosoftProviderAccountId. Hoisting this to a single const (right after you computeproviderEmail) and reusing it in both places would reduce the risk of the two paths diverging in future edits.Neither of these is blocking, but they would tighten up intent and maintainability.
319-325: Consider not surfacing cache/Redis failures as user-visible link failuresThe catch block currently wraps the entire processing pipeline. That means if
setOAuthCodeResult(or a later Redis call) throws after the account has been successfully created/merged in Prisma, the user will be redirected witherror=link_failedeven though their Microsoft account is actually linked.If you want to avoid “false negative” toasts in that situation, you could:
- Wrap the Redis interactions (
setOAuthCodeResult,clearOAuthCode) in their own internal try/catch, logging failures but not changing the user-facing outcome once the DB work has succeeded.- Keep the outer catch primarily for real linking failures (token exchange, profile fetch, Prisma errors).
That would preserve dedupe behavior when Redis is healthy but avoid telling users their link failed when it actually succeeded.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/google/linking/callback/route.ts(4 hunks)apps/web/app/api/outlook/linking/callback/route.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/api/outlook/linking/callback/route.ts (4)
apps/web/utils/redis/oauth-code.ts (4)
getOAuthCodeResult(27-41)acquireOAuthCodeLock(18-25)setOAuthCodeResult(43-53)clearOAuthCode(55-57)apps/web/utils/outlook/constants.ts (1)
OUTLOOK_LINKING_STATE_COOKIE_NAME(1-1)apps/web/utils/prisma-helpers.ts (1)
isDuplicateError(3-12)apps/web/utils/oauth/error-handler.ts (1)
handleOAuthCallbackError(11-25)
apps/web/app/api/google/linking/callback/route.ts (4)
apps/web/utils/redis/oauth-code.ts (4)
getOAuthCodeResult(27-41)acquireOAuthCodeLock(18-25)setOAuthCodeResult(43-53)clearOAuthCode(55-57)apps/web/utils/gmail/constants.ts (1)
GOOGLE_LINKING_STATE_COOKIE_NAME(16-16)apps/web/utils/prisma-helpers.ts (1)
isDuplicateError(3-12)apps/web/utils/oauth/error-handler.ts (1)
handleOAuthCallbackError(11-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (9)
apps/web/app/api/google/linking/callback/route.ts (7)
1-17: LGTM! Imports are clean and appropriate.The new Redis-backed OAuth utilities and duplicate error helper are properly imported and support the deduplication strategy.
40-54: Excellent cache-hit optimization.The cached result check prevents redundant processing of OAuth codes that have already been handled. The reconstruction of redirect parameters and cookie cleanup are correct.
144-201: Excellent race condition handling for concurrent account creation.The duplicate error handling correctly addresses the scenario where concurrent requests attempt to create the same Google account. The logic properly validates that the duplicate account belongs to the target user before continuing.
The edge case where
accountNowis null (account created then immediately deleted) is handled by rethrowing the error, which is acceptable for this unlikely scenario.
203-210: LGTM! Success result caching and redirect are correct.Storing the success result in Redis enables efficient handling of duplicate callback requests, and the redirect properly communicates the outcome to the user.
241-248: LGTM! Consistent success handling pattern.The account merge path follows the same caching and redirect pattern as account creation, ensuring consistent behavior across both flows.
249-259: Critical: Error path correctly clears Redis state.Clearing the OAuth code from Redis on error (line 250) is essential—it ensures that failed attempts are not cached and allows users to retry the OAuth flow. The error handling flow is correct.
19-260: Robust deduplication architecture.The Redis-backed locking and caching strategy effectively prevents race conditions and duplicate OAuth callback processing:
- Per-code locks prevent concurrent processing
- 60-second TTLs provide automatic cleanup
- Cache hits short-circuit redundant requests
- Duplicate error handling addresses concurrent account creation
- Error paths properly clean up Redis state
The implementation is defensive and handles edge cases well.
apps/web/app/api/outlook/linking/callback/route.ts (2)
1-18: Imports correctly wire in state cookie, Redis helpers, and duplicate detectionThe new imports line up with later usage (
OUTLOOK_LINKING_STATE_COOKIE_NAME, Redis OAuth helpers,isDuplicateError) and keep concerns nicely factored into shared utilities. No issues here.
274-281: Success result caching and redirects are consistent and support deduped callbacksSetting the OAuth code result in Redis and then redirecting with a
successquery param in both the “new account created” and “merge” paths is a good match for the earlycachedResultbranch. Subsequent callbacks for the same code will reuse the exact sameparams, and every success path consistently clears the state cookie on the outgoing response.This should address repeated callback behavior and align with the PR’s goal of avoiding spurious error toasts on successful links.
Also applies to: 310-317
1 participant
Summary by CodeRabbit
Refactor
Bug Fixes