Skip to content

Commit d4a8bb9

Browse files
NuwaniFonsekaNuwaniFonseka
committed
Resolve Sonar security and maintainability issues
1 parent 609bb02 commit d4a8bb9

4 files changed

Lines changed: 94 additions & 48 deletions

File tree

‎src/controllers/enrollmentController.js‎

Lines changed: 71 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,26 @@ const {
55
validateCourse,
66
createGradeRecord,
77
} = require("../services/externalServices");
8+
const { isValidObjectId, sanitizeFilter } = require("mongoose");
9+
10+
const SAFE_ID_REGEX = /^[A-Za-z0-9_-]{1,120}$/;
11+
12+
function sanitizeIdentifier(value, fieldName) {
13+
if (typeof value !== "string") {
14+
const err = new Error(`${fieldName} must be a string`);
15+
err.status = 400;
16+
throw err;
17+
}
18+
19+
const trimmed = value.trim();
20+
if (!SAFE_ID_REGEX.test(trimmed)) {
21+
const err = new Error(`${fieldName} has invalid format`);
22+
err.status = 400;
23+
throw err;
24+
}
25+
26+
return trimmed;
27+
}
828

929
function isAdmin(req) {
1030
return (req.user?.role || "").toLowerCase() === "admin";
@@ -15,16 +35,31 @@ function canAccessStudent(req, studentId) {
1535
return req.user?.id && String(req.user.id) === String(studentId);
1636
}
1737

38+
function handleKnownError(res, error, fallbackMessage) {
39+
if (error?.status) {
40+
return res.status(error.status).json({
41+
message: error.message || fallbackMessage,
42+
});
43+
}
44+
return res.status(500).json({
45+
message: fallbackMessage,
46+
error: error.message,
47+
});
48+
}
49+
1850
exports.createEnrollment = async (req, res) => {
1951
try {
20-
const { student_id, course_id } = req.body;
52+
const rawStudentId = req.body?.student_id;
53+
const rawCourseId = req.body?.course_id;
2154

2255
// Validate input
23-
if (!student_id || !course_id) {
56+
if (!rawStudentId || !rawCourseId) {
2457
return res.status(400).json({
2558
message: "student_id and course_id are required",
2659
});
2760
}
61+
const student_id = sanitizeIdentifier(rawStudentId, "student_id");
62+
const course_id = sanitizeIdentifier(rawCourseId, "course_id");
2863

2964
if (!canAccessStudent(req, student_id)) {
3065
return res.status(403).json({
@@ -39,11 +74,11 @@ exports.createEnrollment = async (req, res) => {
3974
await validateCourse(course_id);
4075

4176
// Prevent duplicate enrollment
42-
const existing = await Enrollment.findOne({
77+
const existing = await Enrollment.findOne(sanitizeFilter({
4378
student_id,
4479
course_id,
4580
status: "ACTIVE",
46-
});
81+
}));
4782

4883
if (existing) {
4984
return res.status(409).json({
@@ -91,35 +126,34 @@ exports.createEnrollment = async (req, res) => {
91126
});
92127
}
93128

94-
res.status(500).json({
95-
message: "Internal server error",
96-
error: error.message,
97-
});
129+
return handleKnownError(res, error, "Internal server error");
98130
}
99131
};
100132

101133
exports.getEnrollmentsByStudent = async (req, res) => {
102134
try {
103-
const { studentId } = req.params;
135+
const studentId = sanitizeIdentifier(req.params?.studentId, "studentId");
104136
if (!canAccessStudent(req, studentId)) {
105137
return res.status(403).json({
106138
message: "Access denied for requested student enrollments",
107139
});
108140
}
109141

110-
const enrollments = await Enrollment.find({ student_id: studentId });
142+
const enrollments = await Enrollment.find(sanitizeFilter({ student_id: studentId }));
111143
res.status(200).json(Array.isArray(enrollments) ? enrollments : []);
112144
} catch (error) {
113-
res.status(500).json({
114-
message: "Error fetching enrollments",
115-
error: error.message,
116-
});
145+
return handleKnownError(res, error, "Error fetching enrollments");
117146
}
118147
};
119148

120149
exports.cancelEnrollment = async (req, res) => {
121150
try {
122151
const { id } = req.params;
152+
if (!isValidObjectId(id)) {
153+
return res.status(400).json({
154+
message: "Invalid enrollment id format",
155+
});
156+
}
123157

124158
const enrollment = await Enrollment.findById(id);
125159

@@ -149,44 +183,41 @@ exports.cancelEnrollment = async (req, res) => {
149183
enrollment,
150184
});
151185
} catch (error) {
152-
res.status(500).json({
153-
message: "Error cancelling enrollment",
154-
error: error.message,
155-
});
186+
return handleKnownError(res, error, "Error cancelling enrollment");
156187
}
157188
};
158189

159190
exports.getEnrollmentsByCourse = async (req, res) => {
160191
try {
161-
const { courseId } = req.params;
162-
let query = { course_id: courseId };
192+
const courseId = sanitizeIdentifier(req.params?.courseId, "courseId");
193+
let query = sanitizeFilter({ course_id: courseId });
163194
if (!isAdmin(req)) {
164-
query = { ...query, student_id: req.user?.id };
195+
const userId = sanitizeIdentifier(req.user?.id, "userId");
196+
query = sanitizeFilter({ ...query, student_id: userId });
165197
}
166198
const enrollments = await Enrollment.find(query);
167199
res.status(200).json(Array.isArray(enrollments) ? enrollments : []);
168200
} catch (error) {
169-
res.status(500).json({
170-
message: "Error fetching course roster",
171-
error: error.message,
172-
});
201+
return handleKnownError(res, error, "Error fetching course roster");
173202
}
174203
};
175204

176205
exports.checkEnrollment = async (req, res) => {
177206
try {
178-
const { studentId, courseId } = req.query;
207+
const { studentId: rawStudentId, courseId: rawCourseId } = req.query;
179208

180-
if (!studentId || !courseId) {
209+
if (!rawStudentId || !rawCourseId) {
181210
return res.status(400).json({
182211
message: "studentId and courseId are required as query parameters",
183212
});
184213
}
214+
const studentId = sanitizeIdentifier(rawStudentId, "studentId");
215+
const courseId = sanitizeIdentifier(rawCourseId, "courseId");
185216

186-
const enrollment = await Enrollment.findOne({
217+
const enrollment = await Enrollment.findOne(sanitizeFilter({
187218
student_id: studentId,
188219
course_id: courseId
189-
});
220+
}));
190221

191222
if (!enrollment) {
192223
return res.status(200).json({
@@ -202,17 +233,19 @@ exports.checkEnrollment = async (req, res) => {
202233
});
203234

204235
} catch (error) {
205-
res.status(500).json({
206-
message: "Error checking enrollment validation",
207-
error: error.message,
208-
});
236+
return handleKnownError(res, error, "Error checking enrollment validation");
209237
}
210238
};
211239

212240
exports.updateEnrollmentStatus = async (req, res) => {
213241
try {
214242
const { id } = req.params;
215243
const { status } = req.body;
244+
if (!isValidObjectId(id)) {
245+
return res.status(400).json({
246+
message: "Invalid enrollment id format",
247+
});
248+
}
216249

217250
const validStatuses = ["ACTIVE", "CANCELLED", "WITHDRAWN", "COMPLETED"];
218251
if (!validStatuses.includes(status)) {
@@ -243,22 +276,18 @@ exports.updateEnrollmentStatus = async (req, res) => {
243276
});
244277

245278
} catch (error) {
246-
res.status(500).json({
247-
message: "Error updating enrollment status",
248-
error: error.message,
249-
});
279+
return handleKnownError(res, error, "Error updating enrollment status");
250280
}
251281
};
252282

253283
exports.getAllEnrollments = async (req, res) => {
254284
try {
255-
const query = isAdmin(req) ? {} : { student_id: req.user?.id };
285+
const query = isAdmin(req)
286+
? {}
287+
: sanitizeFilter({ student_id: sanitizeIdentifier(req.user?.id, "userId") });
256288
const enrollments = await Enrollment.find(query).sort({ enrolled_at: -1 }).limit(100);
257289
res.status(200).json(Array.isArray(enrollments) ? enrollments : []);
258290
} catch (error) {
259-
res.status(500).json({
260-
message: "Error fetching all enrollments",
261-
error: error.message,
262-
});
291+
return handleKnownError(res, error, "Error fetching all enrollments");
263292
}
264293
};

‎src/middleware/auth.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ const axios = require("axios");
99
const authenticate = async (req, res, next) => {
1010
const authHeader = req.headers.authorization;
1111

12-
if (!authHeader || !authHeader.startsWith("Bearer ")) {
12+
if (!authHeader?.startsWith("Bearer ")) {
1313
return res.status(401).json({ message: "No token provided" });
1414
}
1515

@@ -39,6 +39,7 @@ const authenticate = async (req, res, next) => {
3939
};
4040
return next();
4141
} catch (error) {
42+
console.error("Token validation failed:", error.message);
4243
return res.status(401).json({ message: "Token validation failed" });
4344
}
4445
};

‎src/server.js‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ app.use(limiter);
2626

2727
const swaggerUi = require("swagger-ui-express");
2828
const YAML = require("yamljs");
29-
const path = require("path");
29+
const path = require("node:path");
3030

3131
// Load Swagger document
3232
const swaggerDocument = YAML.load(path.join(__dirname, "swagger.yaml"));
@@ -69,14 +69,18 @@ if (require.main === module) {
6969
process.exit(1);
7070
}
7171

72-
connectDB().then(() => {
72+
(async () => {
73+
await connectDB();
7374
const PORT = process.env.PORT || 3000;
7475
app.listen(PORT, () => {
7576
console.log(`Enrollment Service running on port ${PORT}`);
7677
console.log("Enrollment Service v4 — PURE ROUTES (NO /api PREFIX)");
7778
});
79+
})().catch((err) => {
80+
console.error("Failed to start server:", err.message);
81+
process.exit(1);
7882
});
7983
}
8084

8185
const Enrollment = require("./models/Enrollment");
82-
module.exports = { app, Enrollment };
86+
module.exports = { app, Enrollment };

‎src/services/externalServices.js‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,16 @@
11
const axios = require("axios");
22

33
const allowMocks = process.env.ALLOW_MOCK_SERVICES === "true";
4+
const SAFE_SEGMENT_REGEX = /^[A-Za-z0-9_-]{1,120}$/;
5+
6+
const sanitizePathSegment = (value, fieldName) => {
7+
if (typeof value !== "string" || !SAFE_SEGMENT_REGEX.test(value.trim())) {
8+
const err = new Error(`${fieldName} has invalid format`);
9+
err.status = 400;
10+
throw err;
11+
}
12+
return encodeURIComponent(value.trim());
13+
};
414

515
const handleServiceFailure = (serviceName, error, mockData) => {
616
if (allowMocks) {
@@ -18,8 +28,9 @@ const handleServiceFailure = (serviceName, error, mockData) => {
1828

1929
const validateStudent = async (student_id) => {
2030
try {
31+
const safeStudentId = sanitizePathSegment(student_id, "student_id");
2132
const response = await axios.get(
22-
`${process.env.STUDENT_SERVICE_URL}/students/${student_id}`
33+
`${process.env.STUDENT_SERVICE_URL}/students/${safeStudentId}`
2334
);
2435
return response.data;
2536
} catch (error) {
@@ -33,8 +44,9 @@ const validateStudent = async (student_id) => {
3344

3445
const validateCourse = async (course_id) => {
3546
try {
47+
const safeCourseId = sanitizePathSegment(course_id, "course_id");
3648
const response = await axios.get(
37-
`${process.env.COURSE_SERVICE_URL}/courses/${course_id}`
49+
`${process.env.COURSE_SERVICE_URL}/courses/${safeCourseId}`
3850
);
3951
return response.data;
4052
} catch (error) {

0 commit comments

Comments
 (0)