Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds OAuth callback deduplication and result cookie handling for Google and Outlook account linking. It introduces helper functions for building success redirects and detecting duplicate callbacks, updates all linking routes to use these helpers, modifies cookie management to track completed OAuth flows, and extends state parsing utilities. Version bumped to v2.19.3. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant OAuth as OAuth Provider
participant AuthUrl as /auth-url Route
participant Callback as /callback Route
participant Helper as Helpers
User->>AuthUrl: Request OAuth link
AuthUrl->>AuthUrl: Delete result cookie
AuthUrl->>AuthUrl: Generate state
AuthUrl->>User: Set state cookie + OAuth URL
User->>OAuth: Click link
OAuth->>Callback: Redirect with code & state
rect rgba(100, 150, 200, 0.2)
Note over Callback: Deduplication Check
Callback->>Helper: checkOAuthCallbackDedupe()
Helper->>Helper: Compare state vs result cookie
alt Duplicate Detected
Helper->>Callback: Return redirect with stored params
Callback->>User: Redirect /accounts
else New Request
Callback->>Callback: Continue processing
end
end
rect rgba(100, 200, 150, 0.2)
Note over Callback: Success Path
Callback->>Helper: buildOAuthSuccessRedirect()
Helper->>Helper: Encode state + params to result cookie
Helper->>Callback: Return response with cookies
Callback->>Callback: Delete state cookie
Callback->>User: Redirect /accounts (result cookie set)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/app/api/google/linking/callback/route.ts (1)
4-7: Dedupe + success redirect wiring looks solid; small invariants/nitsThe new flow (early
checkOAuthCallbackDedupe, thenbuildOAuthSuccessRedirectwith state/result cookies) is coherent and should fix duplicate-callback errors while preserving idempotent UX.A couple of minor points to consider:
receivedStateis read twice (forvalidateOAuthCallbackand again at Lines 51-53). Sincevalidation.successimplies a valid state, a future refactor could havevalidateOAuthCallbackreturn the normalized state so you don’t need to re-read and re-guard it here.stateis only used for the result cookie, which is fine; just be aware that ifvalidateOAuthCallback’s behavior ever changes, the “Missing state parameter after validation” throw is outside thetry/catchand will surface as a 500 viawithErrorrather than going through the OAuth error handler.Nothing blocking here, just possible polish for future changes.
Also applies to: 13-16, 21-30, 51-58, 168-174, 205-211
apps/web/utils/oauth/error-handler.ts (1)
9-10: Centralized error redirect + cookie cleanup looks good; watch redirectUrl mutationThe updated
handleOAuthCallbackError(Lines 9-10,17-18,24-30) correctly:
- Constructs its own
NextResponse.redirect,- Always clears the state cookie, and
- Optionally clears the result cookie when provided, which keeps the dedupe state consistent after failures.
One minor design note:
redirectUrlis mutated viasearchParams.set. Callers should treat theURLthey pass in as per-call and not reuse it elsewhere after this call, otherwise they may see accumulatederror/error_descriptionparams. If that ever becomes an issue, you could switch the parameter to a string and construct a freshURLinternally.Also consider whether you truly want to expose raw
error.messageto the client viaerror_description; if not, mapping unknown/internal errors to a generic user-facing string while logging the detailed message would tighten things up.Also applies to: 17-18, 24-30
apps/web/utils/oauth/callback-helpers.ts (1)
9-48: Dedupe helper logic is correct; consider adding observability and clarifying cookie lifecycle
checkOAuthCallbackDedupe(Lines 9-48) correctly:
- Compares the incoming
stateagainst the parsed result cookie,- Requires non-empty
paramsbefore short-circuiting, and- Reconstructs the
/accountsURL with the cached params while clearing the state cookie.A couple of optional improvements:
- Adding a debug/info log when the dedupe path is hit would make it much easier to reason about repeated provider callbacks in production.
- You currently keep the result cookie after a deduped redirect. That’s sensible for handling multiple quick retries, but if you ever want “use-once” semantics, you could also delete
resultCookieNamehere to avoid long-lived success state. Right nowmaxAgeis already short, so this is non-blocking.Overall, the dedupe behavior matches the new state/result-cookie design.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/api/google/linking/auth-url/route.ts(2 hunks)apps/web/app/api/google/linking/callback/route.ts(5 hunks)apps/web/app/api/outlook/linking/auth-url/route.ts(2 hunks)apps/web/app/api/outlook/linking/callback/route.ts(5 hunks)apps/web/utils/gmail/constants.ts(1 hunks)apps/web/utils/oauth/callback-helpers.ts(1 hunks)apps/web/utils/oauth/error-handler.ts(1 hunks)apps/web/utils/oauth/state.ts(1 hunks)apps/web/utils/outlook/constants.ts(1 hunks)version.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/web/app/api/outlook/linking/auth-url/route.ts (1)
apps/web/utils/outlook/constants.ts (1)
OUTLOOK_LINKING_STATE_RESULT_COOKIE_NAME(2-3)
apps/web/app/api/outlook/linking/callback/route.ts (2)
apps/web/app/api/google/linking/callback/route.ts (1)
GET(18-221)apps/web/app/api/outlook/linking/auth-url/route.ts (1)
GET(24-38)
apps/web/utils/oauth/callback-helpers.ts (1)
apps/web/utils/oauth/state.ts (3)
parseOAuthStateResultCookie(62-95)encodeOAuthStateResultCookie(56-60)oauthStateCookieOptions(34-40)
apps/web/app/api/google/linking/callback/route.ts (4)
apps/web/app/api/google/linking/auth-url/route.ts (1)
GET(31-45)apps/web/utils/oauth/callback-helpers.ts (2)
checkOAuthCallbackDedupe(21-48)buildOAuthSuccessRedirect(62-86)apps/web/utils/gmail/constants.ts (2)
GOOGLE_LINKING_STATE_COOKIE_NAME(16-16)GOOGLE_LINKING_STATE_RESULT_COOKIE_NAME(17-18)apps/web/utils/oauth/error-handler.ts (1)
handleOAuthCallbackError(12-30)
apps/web/app/api/google/linking/auth-url/route.ts (1)
apps/web/utils/gmail/constants.ts (1)
GOOGLE_LINKING_STATE_RESULT_COOKIE_NAME(17-18)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
🔇 Additional comments (2)
apps/web/app/api/google/linking/callback/route.ts (1)
127-130: Cookie cleanup behavior is consistent and correctly aligned with new dedupe flowClearing both
GOOGLE_LINKING_STATE_COOKIE_NAMEandGOOGLE_LINKING_STATE_RESULT_COOKIE_NAME:
- On the
linkingResult.type === "redirect"path (Lines 127-130), and- In the error handler call (Lines 215-218, via
handleOAuthCallbackError)ensures callers don’t get stuck with stale state/result cookies after a redirect or error, which is important now that result cookies drive deduplication.
Given this, the only remaining source of a result cookie is a successful
buildOAuthSuccessRedirectcall, which matches the intended “only cache completed flows” behavior.Also applies to: 215-218
apps/web/utils/oauth/callback-helpers.ts (1)
50-86: Success redirect helper is consistent with state TTL and dedupe design
buildOAuthSuccessRedirect(Lines 50-86) cleanly centralizes the success path:
- Builds
/accountswith arbitrary success params.- Deletes the original state cookie.
- Writes a result cookie (state + params) using
oauthStateCookieOptions, giving it the same security flags and TTL as the original state cookie.This lines up well with
checkOAuthCallbackDedupeand the auth-url routes that clear the result cookie when starting a new flow. No issues from a correctness standpoint.
Summary by CodeRabbit