Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe GET /:id route in src/routes/certificate.route.ts was changed to remove the isAuthenticated middleware, making the certificate retrieval endpoint publicly accessible. The route handler remains the same; no other routes or middleware configurations were modified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Router as Express Router
participant Handler as getCertificateById
rect rgba(200,230,255,0.25)
note over Client,Handler: Before (with auth)
participant Auth as isAuthenticated
Client->>Router: GET /certificate/:id
Router->>Auth: Verify session/token
alt Auth OK
Router->>Handler: getCertificateById(req)
Handler-->>Router: res
Router-->>Client: 200 OK / data
else Auth Fail
Auth-->>Client: 401/403
end
end
rect rgba(200,255,200,0.25)
note over Client,Handler: After (public route)
Client->>Router: GET /certificate/:id
Router->>Handler: getCertificateById(req)
Handler-->>Router: res
Router-->>Client: 200 OK / data or 4xx/5xx
end
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/routes/certificate.route.ts(1 hunks)
| router.get('/:id', (req, res, next) => { | ||
| getCertificateById(req, res).catch(next); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public GET /:id can leak PII and enable ID enumeration—add validation, rate limiting, and ensure sanitized response.
If this endpoint is intentionally public, harden it:
- Validate
:idformat to reject garbage/enumeration attempts. - Add a small per-IP rate limit.
- Ensure
getCertificateByIdreturns only public-safe fields (no email, phone, user object, etc.). If owner-only fields exist, introduce agetPublicCertificateByIdserializer or gate viacertificate.isPublicor a short-lived signed token.
Apply this minimal route hardening (keeps it public) and add helpers shown below:
-router.get('/:id', (req, res, next) => {
- getCertificateById(req, res).catch(next);
-});
+router.get(
+ '/:id',
+ validateCertificateIdParam,
+ publicCertificateLimiter,
+ (req, res, next) => {
+ getCertificateById(req, res).catch(next);
+ }
+);Add these helpers near the top of the file:
import rateLimit from 'express-rate-limit';
const publicCertificateLimiter = rateLimit({
windowMs: 60_000,
max: 60,
standardHeaders: true,
legacyHeaders: false,
});
const validateCertificateIdParam: express.RequestHandler = (req, res, next) => {
const { id } = req.params as { id: string };
// Adjust regex if IDs are UUIDs instead of Mongo ObjectIds
if (!/^[0-9a-fA-F]{24}$/.test(id)) {
return res.status(400).json({ message: 'Invalid certificate id' });
}
next();
};Option (if sensitive fields exist): introduce a public-safe controller and switch the route handler:
- (req, res, next) => {
- getCertificateById(req, res).catch(next);
- }
+ (req, res, next) => {
+ getPublicCertificateById(req, res).catch(next);
+ }Verification script to confirm no PII is returned by the public handler:
I can draft a getPublicCertificateById serializer (whitelisting fields) or wire a signed-token gate if needed.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate controller implementation
rg -nP -C3 --type ts '\bgetCertificateById\b' src | sed -n '1,200p'
# Inspect for potentially sensitive fields in the response payload
rg -nP --type ts -C2 '\bres\.(json|send)\s*\(' src/controllers | sed -n '1,200p'
rg -nP --type ts -C2 '\b(email|phone|address|dob|dateOfBirth|user|profile)\b' src/controllers/certificate.controller.ts || true
# Look for populate/select patterns that might pull user data
rg -nP --type ts -C2 '\bpopulate\(|select\(|projection\b' src/controllers/certificate.controller.ts || trueLength of output: 6361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of getCertificateById including its response handling
sed -n '1,200p' src/controllers/certificate.controller.tsLength of output: 3459
Harden public GET /:id to prevent PII exposure and ID enumeration
The current handler
router.get('/:id', (req, res, next) => {
getCertificateById(req, res).catch(next);
});populates and returns the user’s email and allows unlimited requests against any ObjectId.
Please address the following critical issues:
• src/routes/certificate.route.ts (lines 9–11)
– Add a lightweight UUID/ObjectId‐format validator middleware to catch malformed IDs before invoking the controller.
– Apply a per‐IP rate limiter (e.g. 60 requests/minute) to throttle enumeration attempts.
• src/controllers/certificate.controller.ts (lines 12–16)
– Remove PII from public responses. The existing populate('user', 'name email') leaks email.
– Introduce a getPublicCertificateById that whitelists only non‐sensitive fields (e.g. certificate ID, course title, issue date), omitting email/phone/address.
Suggested diff for route hardening:
-import express from 'express';
+import express from 'express';
+import rateLimit from 'express-rate-limit';
+import { validateObjectIdParam } from '../middlewares/validateObjectIdParam';
router.get('/:id', (req, res, next) => {
getCertificateById(req, res).catch(next);
}); router.get(
- '/:id',
- (req, res, next) => {
- getCertificateById(req, res).catch(next);
- }
+ '/:id',
+ validateObjectIdParam('id'),
+ rateLimit({ windowMs: 60_000, max: 60 }),
+ (req, res, next) => {
+ getPublicCertificateById(req, res).catch(next);
+ }
);And sketch of getPublicCertificateById in src/controllers/certificate.controller.ts:
export const getPublicCertificateById = async (req: Request, res: Response) => {
const { id } = req.params;
// (ID format already validated by middleware)
const cert = await Certificate.findById(id)
.populate('course', 'title');
if (!cert) return res.status(404).json({ message: 'Not found' });
// Only expose non-PII fields
const publicCert = {
id: cert._id,
course: cert.course.title,
issuedAt: cert.issuedAt,
// …other safe fields
};
res.status(200).json(publicCert);
};These changes are critical to avoid leaking emails and to thwart brute-force enumeration of certificate IDs.
1 participant
Summary by CodeRabbit
New Features
Security