Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds JWT-based authentication helpers and middleware, migrates QR token signing and verification to JWTs, returns a session token from QR login, and updates route wiring. The env template and package manifest are updated with the JWT secret and dependencies. ChangesJWT Authentication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 3
🤖 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 `@src/auth/authenticate.ts`:
- Around line 4-17: The authenticate middleware only validates the token and
sets req.userId, but it does not enforce that the request’s patientId belongs to
that user, leaving patient-scoped routes vulnerable when handlers read patientId
from query or path. Update authenticate (or a shared patient-guard used by the
appointment, metrics, and patient routes) to derive the patient identity from
verifyToken and compare/bind it to req.userId before calling next(), so the
controller layer no longer needs to repeat the equality check.
In `@src/auth/jwt.ts`:
- Around line 15-20: The JWT helpers in issueToken and verifyToken are being
reused for both session auth and QR signatures, which lets a QR signature be
accepted as a Bearer token by authenticate(). Split the token flows so session
tokens and QR-signature tokens use separate helpers and/or separate
secrets/payload shapes, then update authenticate() to only accept session-token
verification and explicitly reject QR-token formats.
In `@src/services/qr-token.service.ts`:
- Line 20: The QR token flow is reusing the same JWT helper as session auth and
embedding password-bearing payloads directly into the QR value. Update the
qr-token.service logic around issueToken and authenticate to generate a separate
purpose-scoped, opaque one-time QR token or nonce, keep the password only on the
server, and ensure QR tokens cannot be accepted as Bearer/session JWTs. Make
protected routes and token validation accept only session-purpose tokens, and
adjust any QR payload/signature handling so the QR value never contains reusable
credentials.
🪄 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: 61c2312d-e65a-46a3-9ed9-d1d50d32ec8f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.env.examplepackage.jsonsrc/auth/authenticate.tssrc/auth/jwt.tssrc/constants/email/email.constants.tssrc/controllers/login/login.controller.tssrc/routes/appointments.routes.tssrc/routes/metrics.routes.tssrc/routes/patient.routes.tssrc/services/login.service.tssrc/services/qr-token.service.ts
| .update(`${encodedPayload}.${getKey()}`) | ||
| .digest('base64url') | ||
| .slice(0, SIGNATURE_LENGTH); | ||
| return issueToken(encodedPayload); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Separate QR proof tokens from session bearer tokens and avoid password-bearing QR payloads.
encodedPayload is embedded directly in the QR token and contains password; base64url is only encoding, so anyone with the QR value can decode the credential. Also, signature is produced by the same issueToken helper accepted by authenticate, so the QR signature segment is itself a valid Bearer JWT with userId = encodedPayload. Use an opaque one-time QR nonce or a purpose-scoped QR token, keep the password server-side, and make protected routes accept only session-purpose JWTs.
Also applies to: 35-35, 48-48
🤖 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 `@src/services/qr-token.service.ts` at line 20, The QR token flow is reusing
the same JWT helper as session auth and embedding password-bearing payloads
directly into the QR value. Update the qr-token.service logic around issueToken
and authenticate to generate a separate purpose-scoped, opaque one-time QR token
or nonce, keep the password only on the server, and ensure QR tokens cannot be
accepted as Bearer/session JWTs. Make protected routes and token validation
accept only session-purpose tokens, and adjust any QR payload/signature handling
so the QR value never contains reusable credentials.
| userId?: string; | ||
| } | ||
|
|
||
| export function authenticate(req: AuthRequest, res: Response, next: NextFunction): void { |
There was a problem hiding this comment.
I would go with export const authenticate = () ...
| export function authenticate(req: AuthRequest, res: Response, next: NextFunction): void { | ||
| const header = req.headers.authorization; | ||
| if (!header?.startsWith('Bearer ')) { | ||
| res.status(401).json({ error: 'missing token' }); |
There was a problem hiding this comment.
i think that we have library for status codes instead of that 401, also the error message and the Bearer should be saved inside constants and not hardcoded like this
| return; | ||
| } | ||
| try { | ||
| const { userId } = verifyToken(header.slice(7)); |
There was a problem hiding this comment.
save 7 into constant variable, no magic numbers
| req.userId = userId; | ||
| next(); | ||
| } catch { | ||
| res.status(401).json({ error: 'invalid or expired token' }); |
There was a problem hiding this comment.
Same here status codes library and no hardcoded values
|
|
||
| const TOKEN_TTL = '2h'; | ||
|
|
||
| export function issueToken(userId: string) { |
There was a problem hiding this comment.
The name is a bit confusing, maybe change it into signToken or something that will tell us "this function sighns the token", also lets go with function expression and not function
|
|
||
| const router = Router(); | ||
|
|
||
| router.use(authenticate); |
There was a problem hiding this comment.
Why must you need to use this in every router? cant we use it globally ?
|
|
||
| export const findPatientById = (userId: string): Patient | null => { | ||
| return patients.find((patient) => patient.patientId === userId) ?? null; | ||
| }; |
There was a problem hiding this comment.
find is returning undefined if nothing was found, isnt it enough? why do you need to explicitly return null? this counts as bad practice
There was a problem hiding this comment.
i forgot to remove this function i didnt use it
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)
src/controllers/login/login.controller.ts (1)
67-69: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the QR verify response schema to include
token.This endpoint now returns
token: sessionToken, butsrc/routes/login.routes.tsstill documents the 200 response as onlymessageanduser, so generated API docs/client expectations drift from the actual payload.🤖 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 `@src/controllers/login/login.controller.ts` around lines 67 - 69, The QR verify endpoint in login.controller now returns a token field, but the documented 200 response in login.routes is still missing it. Update the response schema used by the QR verify route to include token alongside message and user, and keep the documented payload aligned with the actual return shape from the login controller.
🧹 Nitpick comments (1)
src/app.ts (1)
42-46: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the protected route boundary explicit per route.
The bare
app.use(authenticate)on Line 42 is order-sensitive. Mountingauthenticatedirectly on the protected routes makes future public/protected route additions harder to misplace.♻️ Proposed refactor
-app.use(authenticate); - -app.use('/api/patients', patientRoutes); -app.use('/api/appointment', appointmentRoutes); -app.use('/api/metrics/', metricsRoutes); +app.use('/api/patients', authenticate, patientRoutes); +app.use('/api/appointment', authenticate, appointmentRoutes); +app.use('/api/metrics/', authenticate, metricsRoutes);🤖 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 `@src/app.ts` around lines 42 - 46, The route protection in app.use(authenticate) is implicit and order-sensitive, so make the protected boundary explicit by attaching authenticate directly to each protected mount point in app.ts. Update the app.use calls for patientRoutes, appointmentRoutes, and metricsRoutes so each one is wrapped or chained with authenticate, and remove the standalone global authenticate middleware to avoid accidentally protecting future public routes.
🤖 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 `@src/auth/authenticate.ts`:
- Around line 18-35: The authenticate flow is conflating JWT validation failures
with database lookup failures, so any dbService.get or patientModel access error
is incorrectly returned as INVALID_OR_EXPIRED_TOKEN. Update authenticate to
catch verifyToken errors separately from the patient lookup around
dbService.get(patientModel, { patientId: userId }), keep the 401 response only
for missing/invalid tokens, and send lookup/service errors to the next error
handler instead of swallowing them in the same catch.
---
Outside diff comments:
In `@src/controllers/login/login.controller.ts`:
- Around line 67-69: The QR verify endpoint in login.controller now returns a
token field, but the documented 200 response in login.routes is still missing
it. Update the response schema used by the QR verify route to include token
alongside message and user, and keep the documented payload aligned with the
actual return shape from the login controller.
---
Nitpick comments:
In `@src/app.ts`:
- Around line 42-46: The route protection in app.use(authenticate) is implicit
and order-sensitive, so make the protected boundary explicit by attaching
authenticate directly to each protected mount point in app.ts. Update the
app.use calls for patientRoutes, appointmentRoutes, and metricsRoutes so each
one is wrapped or chained with authenticate, and remove the standalone global
authenticate middleware to avoid accidentally protecting future public routes.
🪄 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: d79460d4-1dc9-4142-9f1f-ff4ce53d2ff8
📒 Files selected for processing (6)
src/app.tssrc/auth/authenticate.tssrc/auth/jwt.tssrc/constants/auth.constants.tssrc/controllers/login/login.controller.tssrc/services/qr-token.service.ts
✅ Files skipped from review due to trivial changes (1)
- src/constants/auth.constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/qr-token.service.ts
| try { | ||
| const { userId } = verifyToken(header.slice(BEARER_PREFIX.length)); | ||
| const [patient] = await dbService.get(patientModel, { patientId: userId }); | ||
|
|
||
| if (!patient) { | ||
| res | ||
| .status(StatusCodes.UNAUTHORIZED) | ||
| .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN }); | ||
| return; | ||
| } | ||
|
|
||
| req.userId = userId; | ||
| next(); | ||
| } catch { | ||
| res | ||
| .status(StatusCodes.UNAUTHORIZED) | ||
| .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t return token-invalid 401s for DB lookup failures.
Line 20 is inside the same catch as JWT verification, so a database/service error is reported as INVALID_OR_EXPIRED_TOKEN. Catch verification errors separately and pass lookup failures to the error handler.
🐛 Proposed fix
- try {
- const { userId } = verifyToken(header.slice(BEARER_PREFIX.length));
- const [patient] = await dbService.get(patientModel, { patientId: userId });
+ let userId: string;
+
+ try {
+ ({ userId } = verifyToken(header.slice(BEARER_PREFIX.length)));
+ } catch {
+ res
+ .status(StatusCodes.UNAUTHORIZED)
+ .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN });
+ return;
+ }
+
+ try {
+ const [patient] = await dbService.get(patientModel, { patientId: userId });
if (!patient) {
res
.status(StatusCodes.UNAUTHORIZED)
.json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN });
@@
req.userId = userId;
next();
- } catch {
- res
- .status(StatusCodes.UNAUTHORIZED)
- .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN });
+ } catch (error) {
+ next(error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const { userId } = verifyToken(header.slice(BEARER_PREFIX.length)); | |
| const [patient] = await dbService.get(patientModel, { patientId: userId }); | |
| if (!patient) { | |
| res | |
| .status(StatusCodes.UNAUTHORIZED) | |
| .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN }); | |
| return; | |
| } | |
| req.userId = userId; | |
| next(); | |
| } catch { | |
| res | |
| .status(StatusCodes.UNAUTHORIZED) | |
| .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN }); | |
| } | |
| let userId: string; | |
| try { | |
| ({ userId } = verifyToken(header.slice(BEARER_PREFIX.length))); | |
| } catch { | |
| res | |
| .status(StatusCodes.UNAUTHORIZED) | |
| .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN }); | |
| return; | |
| } | |
| try { | |
| const [patient] = await dbService.get(patientModel, { patientId: userId }); | |
| if (!patient) { | |
| res | |
| .status(StatusCodes.UNAUTHORIZED) | |
| .json({ error: AUTH_ERROR_MESSAGES.INVALID_OR_EXPIRED_TOKEN }); | |
| return; | |
| } | |
| req.userId = userId; | |
| next(); | |
| } catch (error) { | |
| next(error); | |
| } |
🤖 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 `@src/auth/authenticate.ts` around lines 18 - 35, The authenticate flow is
conflating JWT validation failures with database lookup failures, so any
dbService.get or patientModel access error is incorrectly returned as
INVALID_OR_EXPIRED_TOKEN. Update authenticate to catch verifyToken errors
separately from the patient lookup around dbService.get(patientModel, {
patientId: userId }), keep the 401 response only for missing/invalid tokens, and
send lookup/service errors to the next error handler instead of swallowing them
in the same catch.
| app.use('/api/tips', tipsRoutes); | ||
| app.use('/api/metrics/', metricsRoutes); | ||
| app.use('/api/email', emailRoutes); |
There was a problem hiding this comment.
tips and email route should be also protected by this middleware
| return parsePayload(encodedPayload); | ||
| } catch { | ||
| } catch (error) { | ||
| console.error('Failed to verify QR token:', error); |
There was a problem hiding this comment.
I think we have a logger for this
There was a problem hiding this comment.
i couldt find a logger for this
|
|
||
| export const JWT_SECRET = process.env.JWT_SECRET; | ||
|
|
||
| const TOKEN_TTL = '2h'; |
There was a problem hiding this comment.
consider to make it an env variable so we can easily change it by the hospital request
3 participants
Description
jwt
Please include a summary of the changes and the related issue.
Related Issue(s)
Fixes # (issue number)
Checklist:
Screenshots (if appropriate):
Summary by CodeRabbit
token) on successful verification.JWT_SECRET; added required JWT dependencies.patientIdoptional forGET /api/patients/:patientId.