Skip to content

Conversation

@hlbmtc
Copy link
Contributor

@hlbmtc hlbmtc commented Jan 27, 2026

Previous PR broke logic of legacy token exchange. This PR simplifies JWT tokens lifecycle checks and fixed migration

Summary by CodeRabbit

  • Refactor

    • Streamlined middleware authentication flow to verify, refresh, and clean up tokens in a single, predictable sequence.
    • Simplified legacy token migration to run only as a fallback.
  • Bug Fixes

    • Improved error handling to preserve sessions on transient errors and avoid unnecessary token clearing.
    • More reliable token refresh behavior when refresh tokens are present.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

Reworks middleware token handling: verification now returns boolean, refresh always attempts when refresh token exists, transient errors preserve tokens, legacy migration is a fallback, and token cleanup is consolidated into a single sequential flow before locale handling. ApiError is introduced for client-error classification.

Changes

Cohort / File(s) Summary
Authentication middleware refactor
front_end/src/middleware.ts
Replaced verifyToken(responseAuth): Promise<void> with verifyToken(): Promise<boolean>; renamed refreshTokensIfNeeded(...)refreshTokens(...) and changed semantics to always attempt refresh if a refresh token exists (return true on success, false on definitive client errors, throw on transient errors); removed proactive in-middleware refresh path and legacy early migration short-circuit; added ApiError import and transient-error handling to preserve tokens.
Legacy token migration simplification
front_end/src/services/auth_tokens_migration.ts
Removed AuthCookieReader import and parameter; dropped early short-circuit that returned when a legacy session existed; kept legacy exchange/fetch, 400 handling (delete legacy cookie), and responseAuth update logic.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Middleware
  participant TokenVerifyAPI as "Verify API"
  participant TokenRefreshAPI as "Refresh API"
  participant CookieMgr as "AuthCookieManager"

  Client->>Middleware: request (cookies: access?, refresh?, legacy?)
  Middleware->>CookieMgr: read access token
  alt access token exists and not expired
    Middleware->>TokenVerifyAPI: verify access token
    TokenVerifyAPI-->>Middleware: valid / invalid
    alt valid
      Middleware->>CookieMgr: keep tokens (session established)
    else invalid
      Middleware->>CookieMgr: read refresh token
      alt refresh token exists
        Middleware->>TokenRefreshAPI: refresh tokens
        TokenRefreshAPI-->>Middleware: success / client-error / transient-error
        alt success
          Middleware->>CookieMgr: apply new tokens
        else client-error
          Middleware->>CookieMgr: clear JWT & legacy tokens
        else transient-error
          Middleware->>CookieMgr: preserve existing tokens (log)
        end
      else no refresh token
        Middleware->>CookieMgr: clear JWT & legacy tokens (if any)
      end
    end
  else no access token
    Middleware->>CookieMgr: read refresh token
    alt refresh token exists
      Middleware->>TokenRefreshAPI: refresh tokens
      TokenRefreshAPI-->>Middleware: success / client-error / transient-error
      alt success
        Middleware->>CookieMgr: apply new tokens
      else client-error
        Middleware->>CookieMgr: clear JWT & legacy tokens
      else transient-error
        Middleware->>CookieMgr: preserve existing tokens (log)
      end
    else no tokens
      Middleware->>CookieMgr: attempt legacy migration (fallback)
    end
  end
  Middleware->>Client: continue (locale handling / response)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • cemreinanc

Poem

🐰 I hopped through code with careful paws,

Cleared old crumbs and tightened laws,
Tokens refreshed, then tested true,
Legacy left with morning dew.
A tidy trail — a rabbit's muse. 🥕

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main changes: simplifying JWT token verification and fixing legacy token migration in Next.js middleware.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
front_end/src/middleware.ts (1)

16-22: Consider making the 4xx check more explicit.

The docstring states this returns true for 4xx errors, but the implementation error.response.status < 500 technically includes 1xx-3xx as well. While ApiError is likely only thrown for 4xx/5xx responses, a more explicit check would improve clarity and defensiveness.

Suggested change
 function isClientError(error: unknown): boolean {
-  return ApiError.isApiError(error) && error.response.status < 500;
+  return ApiError.isApiError(error) && error.response.status >= 400 && error.response.status < 500;
 }
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b47756 and 72b494b.

📒 Files selected for processing (1)
  • front_end/src/middleware.ts
⏰ 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). (4)
  • GitHub Check: Build Docker Image / Build Docker Image
  • GitHub Check: Frontend Checks
  • GitHub Check: integration-tests
  • GitHub Check: Backend Checks
🔇 Additional comments (5)
front_end/src/middleware.ts (5)

24-47: LGTM!

The error handling properly distinguishes between definitive client errors (4xx → return false) and transient errors (5xx/network → throw), allowing the caller to decide whether to clear tokens or preserve them.


77-99: Good fix for the transient error handling.

This properly addresses the previous concern about clearing tokens on 5xx/network errors. The flow now:

  • Returns false on 4xx → tokens cleared (definitive failure)
  • Throws on 5xx/network → caught at line 96, tokens preserved

101-109: Note: Legacy migration won't run on the same request where JWT tokens are cleared.

The condition on line 103 uses the original accessToken/refreshToken values from the request (lines 78-79), not the cleared response state. If a user has invalid JWT tokens that result in 4xx errors, tokens get cleared but legacy migration won't run until the next request.

This is likely intentional to avoid mixing token flows in a single request, but worth confirming this one-request delay is acceptable for the migration period.


111-148: LGTM!

The authentication required check, alpha access handling, and locale propagation logic remain unchanged and function correctly with the new token handling flow.


54-56: No action required. ServerAuthApi.verifyToken() correctly accesses the token through the server-side fetch chain, which automatically reads tokens from request cookies via getAuthCookieManager(). While the access token is extracted in the middleware to check expiration, verifyToken() does not need it passed as a parameter because the underlying serverFetcherserverAppFetchserverFetch chain automatically retrieves and injects the token from cookies into the Authorization header. This is the standard pattern for Next.js server-side authentication.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@ncarazon ncarazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@front_end/src/middleware.ts`:
- Around line 60-89: The code currently unconditionally clears tokens when
verifyToken() or refreshTokens() returns false; change this so tokens are only
cleared when the underlying API indicates explicit invalid credentials (e.g.,
400/401) and preserved on transient/network/5xx errors. Update verifyToken() and
refreshTokens(requestAuth, responseAuth) to return a richer result (e.g., { ok:
boolean, invalidCredentials?: boolean } or throw errors with a
status/statusCode) and use that result here to only call
responseAuth.clearAuthTokens() and response.cookies.delete("auth_token") when
invalidCredentials is true; otherwise leave tokens intact and treat the check as
a transient failure. Also ensure handleLegacyTokenMigration is only attempted
when tokens truly do not exist or have been explicitly invalidated.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3615ea6 and 2b47756.

📒 Files selected for processing (2)
  • front_end/src/middleware.ts
  • front_end/src/services/auth_tokens_migration.ts
🧰 Additional context used
🧬 Code graph analysis (1)
front_end/src/middleware.ts (3)
front_end/src/services/api/auth/auth.server.ts (2)
  • refreshTokens (29-36)
  • verifyToken (54-56)
front_end/src/services/auth_tokens.ts (2)
  • AuthCookieReader (82-114)
  • AuthCookieManager (120-156)
front_end/src/services/auth_tokens_migration.ts (1)
  • handleLegacyTokenMigration (20-54)
⏰ 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). (4)
  • GitHub Check: Build Docker Image / Build Docker Image
  • GitHub Check: integration-tests
  • GitHub Check: Frontend Checks
  • GitHub Check: Backend Checks
🔇 Additional comments (3)
front_end/src/services/auth_tokens_migration.ts (1)

10-10: Import cleanup looks good.

Removing AuthCookieReader from the import list matches the updated function signature and keeps the dependency surface tidy.

front_end/src/middleware.ts (2)

15-34: LGTM — refresh path now applies tokens to the response.

This aligns with the new flow and keeps the mutation localized to responseAuth.


36-47: Auth forwarding is already implemented—no action needed.

The access token is automatically forwarded as an Authorization header. When verifyToken() calls ServerAuthApi.verifyToken(), the serverFetch function retrieves the token via await getAuthCookieManager() and adds it to the request as Authorization: Bearer <token>. This works correctly in the middleware async context with Next.js 15.2.4.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 27, 2026

🧹 Preview Environment Cleaned Up

The preview environment for this PR has been destroyed.

Resource Status
🌐 Preview App ✅ Deleted
🗄️ PostgreSQL Branch ✅ Deleted
⚡ Redis Database ✅ Deleted
🔧 GitHub Deployments ✅ Removed
📦 Docker Image ⚠️ Retained (auto-cleanup via GHCR policies)

Cleanup triggered by PR close at 2026-01-27T17:17:49Z

@hlbmtc hlbmtc merged commit de54e7e into main Jan 27, 2026
14 checks passed
@hlbmtc hlbmtc deleted the fix/middleware-auth-token-migration branch January 27, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants