Fix: Prevent revoked refresh tokens from bypassing silent refresh#211
Conversation
|
Someone is attempting to deploy a commit to the Kunal Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
🎉 Welcome to CodeLens — Thank You for Your Contribution!Hey @riya-chauhan12! 👋 We are genuinely excited to have you here. Every single PR — big or small — makes CodeLens better, and yours is no exception. Take a moment to review the checklist below to help us merge your work quickly and smoothly. ✅ Before Requesting a Review
💬 Join Our Community Channel — This is MandatoryBeing part of our communication channel is compulsory for all contributors, not optional. Why join? This is where all important announcements, PR review updates, contribution discussions, and maintainer decisions happen in real time. Contributors who are not in the channel regularly miss critical context and updates, which often leads to duplicated or misaligned work. Staying connected here is what keeps the community strong and your contributions impactful. We are rooting for you! If you have any questions, drop them in the channel or comment right here on this PR. Let's build something great together. 🚀✨ |
|
Warning Review limit reached
More reviews will be available in 40 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe auth middleware imports ChangesSilent Refresh Token Revocation Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/middlewares/authMiddleware.js (2)
71-78: 💤 Low valueInconsistent formatting reduces readability.
The object literals have irregular indentation and spacing (e.g.,
refreshUser._id ,emailwith extra space before comma, inconsistent line breaks). Consider reformatting for consistency with the rest of the codebase.✨ Suggested formatting
- const newAccessToken = generateAccessToken({userId: refreshUser._id, email: refreshUser.email, role: refreshUser.role - }); - - // Set only the new access token cookie - setAccessTokenCookie(res, newAccessToken); - - decoded = {userId: refreshUser._id ,email: refreshUser.email, role: refreshUser.role - }; + const newAccessToken = generateAccessToken({ + userId: refreshUser._id, + email: refreshUser.email, + role: refreshUser.role + }); + + // Set only the new access token cookie + setAccessTokenCookie(res, newAccessToken); + + decoded = { + userId: refreshUser._id, + email: refreshUser.email, + role: refreshUser.role + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/middlewares/authMiddleware.js` around lines 71 - 78, The object literals in the generateAccessToken function call and the decoded variable assignment have inconsistent formatting with irregular spacing and indentation. Reformat both the object passed to generateAccessToken and the decoded object to have consistent spacing around commas and colons, and remove the extra space before commas (e.g., in refreshUser._id ,email). Ensure the formatting matches the style conventions used elsewhere in the codebase.
65-70: ⚡ Quick winConsider clearing the refresh token hash on invalid token to match explicit endpoint behavior.
The explicit
/api/auth/refreshendpoint (context snippet 2, lines 163-164) clearssecurity.refreshTokenHashand auth cookies when an invalid token is presented, treating it as potential token theft. This middleware only rejects with 401 but leaves the session intact.An attacker with a stolen/rotated token could repeatedly probe silent refresh without triggering session invalidation. Aligning behavior would strengthen the defense-in-depth.
🛡️ Suggested enhancement
if (!isValid) { + await User.findByIdAndUpdate(userId, { $unset: { "security.refreshTokenHash": "" } }); throw new ApiError( 401, "Invalid session. Please log in again." ); }Note: You may also want to import and call
clearAuthCookies(res)before throwing, consistent with the explicit refresh endpoint's defensive behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/middlewares/authMiddleware.js` around lines 65 - 70, In the authMiddleware where the invalid session check throws the ApiError for "Invalid session. Please log in again.", you need to align the behavior with the explicit refresh endpoint by clearing the session state before throwing. Before the throw statement in the if (!isValid) block, clear the refreshTokenHash from the security object and call clearAuthCookies(res) to invalidate the auth cookies, matching the defensive behavior of the /api/auth/refresh endpoint and preventing repeated probing with stolen tokens.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/middlewares/authMiddleware.js`:
- Around line 71-78: The object literals in the generateAccessToken function
call and the decoded variable assignment have inconsistent formatting with
irregular spacing and indentation. Reformat both the object passed to
generateAccessToken and the decoded object to have consistent spacing around
commas and colons, and remove the extra space before commas (e.g., in
refreshUser._id ,email). Ensure the formatting matches the style conventions
used elsewhere in the codebase.
- Around line 65-70: In the authMiddleware where the invalid session check
throws the ApiError for "Invalid session. Please log in again.", you need to
align the behavior with the explicit refresh endpoint by clearing the session
state before throwing. Before the throw statement in the if (!isValid) block,
clear the refreshTokenHash from the security object and call
clearAuthCookies(res) to invalidate the auth cookies, matching the defensive
behavior of the /api/auth/refresh endpoint and preventing repeated probing with
stolen tokens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4000dcac-fa7a-4a40-8065-0c2a12ecc1bf
📒 Files selected for processing (1)
server/middlewares/authMiddleware.js
|
@coderabbitai please check the work done by @riya-chauhan12 also check is this fully compatible with our codebase and especially nearby files I want strict check because this project is deployed on vercel and render so after merging , no error in production must come so check in deep and tell whether this PR is good to good and hence ready for merge and if not then tell in detail what more contributor has to do more ? |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the PR details and diff
gh pr view 211 --repo kunalverma2512/CodeLens --json title,body,state,files,additions,deletionsLength of output: 3845 🏁 Script executed: #!/bin/bash
# Get PR diff
gh pr diff 211 --repo kunalverma2512/CodeLensLength of output: 2893 🏁 Script executed: #!/bin/bash
# Read the authMiddleware.js
cat server/middlewares/authMiddleware.jsLength of output: 3978 🏁 Script executed: #!/bin/bash
# Read nearby auth-related files
cat server/routes/auth.js 2>/dev/null || echo "NOT FOUND"
fd -t f "auth" --extension js --extension ts | head -20Length of output: 287 🏁 Script executed: #!/bin/bash
# Get issue `#208` details
gh issue view 208 --repo kunalverma2512/CodeLens --json title,body,commentsLength of output: 31897 🏁 Script executed: #!/bin/bash
# Find and read test files for authMiddleware
fd -t f "authMiddleware" | head -10
fd -t f -g "*auth*test*" -o -g "*test*auth*" | head -10
fd -t f "test" --extension js --extension ts | xargs grep -l "authMiddleware\|silent.refresh\|refreshToken" 2>/dev/null | head -10Length of output: 479 🏁 Script executed: #!/bin/bash
# Read user model to understand security.refreshTokenHash structure
fd -t f "User" --extension js --extension ts | head -5
fd -t f "user" --extension js --extension ts | xargs grep -l "refreshTokenHash\|security" 2>/dev/null | head -5Length of output: 335 🏁 Script executed: #!/bin/bash
# Check the auth refresh route
fd -t f --extension js --extension ts | xargs grep -l "refreshTokenHash" 2>/dev/null | head -10Length of output: 276 🏁 Script executed: #!/bin/bash
cat server/modules/auth/controller.jsLength of output: 10689 🏁 Script executed: #!/bin/bash
cat server/utils/tokenHelper.js
cat server/models/User.jsLength of output: 5518
✅ What's Correct (the security core)The fundamental fix is directionally right. The contributor correctly identified and patched the root cause: the silent refresh path now performs the same four-step validation as
This is the right approach and addresses the issue at its root. ❌ Issues That Must Be Fixed Before Merging1. 🔴 Missing
|
| Issue | Severity | Required? |
|---|---|---|
Missing clearAuthCookies (revoked + invalid paths) |
🔴 Critical | Yes |
| No test files committed | 🔴 Critical | Yes |
| Double DB query on silent refresh | 🟡 Should fix | Strongly recommended |
| Indentation/formatting | 🟡 Should fix | Yes for a production codebase |
@riya-chauhan12 please address the above four points before this PR is merged. The core logic is solid — these are the remaining gaps to bring it to production quality. Specifically, the clearAuthCookies + optional DB $unset calls on the rejection paths are the most important change, as they mirror the contract already established in controller.js.
🐇 (\\_/) 🔍
( •.•) — almost there, a few more hops needed
|
Hey @riya-chauhan12, Great work identifying and tackling this correctly — the core logic of your fix is right and your understanding of the problem is solid. You've successfully added the DB hash validation that was missing. Before I merge this, there are 3 specific things you need to address. Please read all of them carefully and understand why each one matters before touching any code. 🔴 Issue 1 — Missing
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/middlewares/authMiddleware.js`:
- Line 53: The sensitive `security.refreshTokenHash` field is being fetched and
cached as `prefetchedUser` which could expose this data in logs or serialization
downstream. After fetching the user document with the refreshTokenHash selector
at the locations where
`User.findById(userId).select("+security.refreshTokenHash")` is called, convert
the user document to a plain object using `.toObject()` and delete the
refreshTokenHash property before assigning it to `prefetchedUser`. This ensures
the sensitive hash is not cached on `req.user`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f1b3def-0754-402f-8ab3-610b8bba1e89
📒 Files selected for processing (1)
server/middlewares/authMiddleware.js
|
@coderabbitai please check recent 3 commits and final one and tell whether final commit is well and good and ready to merge |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get recent commits on the PR
gh pr view 211 --repo kunalverma2512/CodeLens --json commits --jq '.commits[-5:]'Length of output: 1428 🏁 Script executed: #!/bin/bash
# Get the current diff of the PR
gh pr diff 211 --repo kunalverma2512/CodeLensLength of output: 5335 🏁 Script executed: #!/bin/bash
# Read the current authMiddleware.js
cat server/middlewares/authMiddleware.jsLength of output: 4559 🏁 Script executed: #!/bin/bash
# Check for test files
fd -t f -e js -e ts | xargs grep -l "authMiddleware\|silent.refresh\|refreshToken" 2>/dev/null | head -20
fd -t f -e test.js -e spec.js -e test.ts -e spec.ts | head -20
find . -name "*.test.*" -o -name "*.spec.*" | grep -i "auth" | head -20Length of output: 662 🏁 Script executed: #!/bin/bash
# Read tokenHelper.js to verify clearAuthCookies
cat server/utils/tokenHelper.jsLength of output: 2956 🏁 Script executed: #!/bin/bash
# Read controller.js for comparison
cat server/modules/auth/controller.jsLength of output: 10725
✅ Issues 1 and 3 — Fully Fixed
This now matches the Double DB query eliminated (was 🟡): ❌ Issue 2 — Tests Still Not CommittedThe repo has zero test files of any kind. This is a project-wide gap, not just this PR. The PR description mentions local regression tests but they were never committed. For a security-critical fix on a production deployment (Render), this is still a risk — there's nothing in CI to catch regressions. This is the single most important thing still missing. 🟡 Issue 4 — Formatting Partially Improved but Still BrokenThe code logic is entirely correct, but the indentation structure of the outer try-catch block was shifted 2 spaces in the wrong direction in the latest commits. Specifically: // ❌ These lines are at 6-space indent — should be 4:
const userId = decoded.userId || decoded.id || decoded._id;
if (!userId) { ... }
const user = prefetchedUser ?? ...
req.user = user;
next();
// ❌ Outer catch is at 4-space indent — should be 2:
} catch (error) {
// ❌ Closing `});` is at 5 spaces — should be 6:
});
// ❌ Arrow function closing `};` is at 2 spaces — should be 0:
};Also a trailing space on the Final Verdict
The security fix is now complete and correct. The Before merging, I'd recommend:
The core security work is done well. It's one formatting cleanup away from being merge-ready. 🐇 |
|
@riya-chauhan12 after your commits , ping me first and make sure you never apply changes or modification made my coderabbitai when it runs automatically after your commit because it might not review nicely After your commit , ping me then I will ping coderabbitai and then study coderabbitai suggestions |
|
@riya-chauhan12 dont work on test files now and leave that and please see no unneccessary spaces, tab space etc exist in files Use prettier extension for formatting the file so that it follow uniformity |
|
Hi @kunalverma2512, I've pushed the requested changes and addressed the review points:
I also addressed the latest CodeRabbit feedback regarding I've verified the updated behavior locally and will wait for your review before making any further changes. Please let me know if there is anything else you would like adjusted. Thanks again for the review and guidance. |
@riya-chauhan12 please let me know that have you understood what I said in previous comments ? |
|
@coderabbitai please check @riya-chauhan12 latest commit regarding security.refreshTokenHash as when I asked before than you didnt pointed me for this so is this commit is well and good and improves backend or its not needed ? please check coding done by @riya-chauhan12 and tell finally whats remaining ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get recent commits on the PR
gh pr view 211 --repo kunalverma2512/CodeLens --json commits --jq '.commits[-5:]'Length of output: 1428 🏁 Script executed: #!/bin/bash
# Get current diff
gh pr diff 211 --repo kunalverma2512/CodeLensLength of output: 5321 🏁 Script executed: #!/bin/bash
# Read current authMiddleware.js
cat server/middlewares/authMiddleware.jsLength of output: 4555 🏁 Script executed: #!/bin/bash
# Read tokenHelper.js
cat server/utils/tokenHelper.jsLength of output: 2929 🏁 Script executed: #!/bin/bash
# Read controller.js for reference comparison
cat server/modules/auth/controller.jsLength of output: 10735 🏁 Script executed: #!/bin/bash
cat server/models/User.jsLength of output: 2667
About the Latest Commit: "fix: remove refresh token hash from prefetched user"Is it needed? Yes — it's a valid and good defensive security measure. Here's why: the silent refresh query uses Any downstream route handler that does something like if (refreshUser.security) {
refreshUser.security.refreshTokenHash = undefined;
}
prefetchedUser = refreshUser;This is the right approach and matches the same hygiene principle used elsewhere in the codebase (e.g., Full Scorecard — Final State
Final VerdictThe security core is complete, correct, and production-safe. The four critical issues from the original review are all addressed. The latest commit is a net improvement — not unnecessary work. The PR closes the revocation bypass described in Ready to merge once you're satisfied with formatting. 🐇 |
|
@riya-chauhan12 Outstanding work! 🎉 You didn't just fix the surface bug you thought through the full chain of implications. You identified the root cause, implemented the correct four-step validation, handled cookie cleanup on both rejection paths, eliminated the double DB query, and then on your own initiative caught and fixed the hash leakage risk on Merging this PR now. Well done! 🧠 What you can take away from this PR and where to grow next: This PR touched one of the most nuanced areas in backend development the gap between cryptographic validity and application-level trust. You learned that a JWT being structurally valid is never enough on its own; the database is the ground truth for session state. From here, I'd encourage you to go deeper into stateless vs. stateful session management, study how refresh token rotation and family-based revocation work (where a single stolen token invalidates an entire device family), explore OWASP's session management guidelines, and look into how production systems like GitHub and Stripe handle token lifecycle at scale. Understanding |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
Fix: Prevent revoked refresh tokens from bypassing silent refresh
Related Issue
Closes #208
Summary
This PR fixes a security issue in the authentication middleware where revoked refresh tokens could still be used during the silent refresh flow.
Problem
The
/api/auth/refreshendpoint correctly validates refresh tokens against the storedsecurity.refreshTokenHashbefore issuing new tokens.However, the silent refresh logic inside
authMiddleware.jsonly verified the JWT signature usingverifyRefreshToken()and did not validate the presented refresh token against the hash stored in the database.As a result, if an access token expired and a revoked refresh token was still present, the middleware could silently authenticate the request and issue a new access token.
Root Cause
The silent refresh path performed:
but skipped:
security.refreshTokenHashThis created inconsistent behavior between:
/api/auth/refreshauthMiddlewareSolution
Added refresh token hash validation to the silent refresh flow.
The middleware now:
security.refreshTokenHashbcrypt.compare()401 UnauthorizedresponseThe new access token is only issued when the refresh token successfully passes hash validation.
Verification
To reproduce and validate the issue, I created a local regression test covering the silent refresh flow.
Test Case 1: Revoked Refresh Token
Setup:
Created a test user
Generated a refresh token
Removed
security.refreshTokenHashto simulate logout/session revocationSent a request with:
Result before fix:
200 OKResult after fix:
401 UnauthorizedTest Case 2: Valid Refresh Token
Setup:
Created a test user
Stored a valid refresh token hash
Sent a request with:
Result:
200 OKFiles Changed
server/middlewares/authMiddleware.jsImpact
This change ensures refresh token revocation is consistently enforced across all authentication flows and prevents revoked refresh tokens from being used to regain access through silent refresh.
Summary by CodeRabbit
Bug Fixes