Skip to content

New Pull Request for Corrected Commit withtout corrupt files issue #2792 #3083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MomoWorlData wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

New Pull Request for Corrected Commit withtout corrupt files issue #2792 #3083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bc1b54
Corrected Commit withtout corrupt files
Dec 9, 2025
6f08e6f
Empty commit issuse #2792
Dec 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/src/config/routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import QueueRoutes from "../routes/v1/queueRoute.js";
import LogRoutes from "../routes/v1/logRoutes.js";
import DiagnosticRoutes from "../routes/v1//diagnosticRoute.js";
import NotificationRoutes from "../routes/v1/notificationRoute.js";
import SummaryRoutes from "../routes/v1/summaryRoute.js"; //for Homepage widget

Comment on lines 14 to 16
Copy link

@llamapreview llamapreview bot Dec 9, 2025

Choose a reason for hiding this comment

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

P1 | Confidence: High

The route is mounted incorrectly, causing it to bypass all existing route configuration and middleware. The app.use(SummaryRoutes) without a path prefix means the router's internal path (/api/v1/summary) is mounted at the root, creating a route at //api/v1/summary (double slash) which is malformed. More critically, it bypasses the verifyJWT middleware that protects all other /api/v1/* routes, making this endpoint publicly accessible without authentication. This violates the established security pattern of the application where all /api/v1/ endpoints (except public ones like /api/v1/status-page) require authentication. This creates a security vulnerability by exposing potentially sensitive system metrics without authentication, and breaks architectural consistency.

  • Speculative: The route registration is inserted between /api/v1/diagnostic and /api/v1/incidents routes without following the established alphabetical or logical grouping pattern. Other routes are imported and registered in a consistent order. While not functionally incorrect, this inconsistency makes the code harder to maintain and scan. Following the established pattern improves code readability and maintainability.
Suggested change
import NotificationRoutes from "../routes/v1/notificationRoute.js";
import SummaryRoutes from "../routes/v1/summaryRoute.js"; //for Homepage widget
app.use("/api/v1/summary", verifyJWT, SummaryRoutes);
// Also ensure the import is placed in alphabetical order with other imports.

Evidence: path:server/src/config/routes.js, path:server/src/routes/v1/summaryRoute.js, search:app.use, search:SummaryRoutes

import IncidentRoutes from "../routes/v1/incidentRoute.js";

Expand Down Expand Up @@ -42,4 +43,5 @@ export const setupRoutes = (app, controllers) => {
app.use("/api/v1/notifications", verifyJWT, notificationRoutes.getRouter());
app.use("/api/v1/diagnostic", verifyJWT, diagnosticRoutes.getRouter());
app.use("/api/v1/incidents", verifyJWT, incidentRoutes.getRouter());
app.use(SummaryRoutes); //used for Homepage widget
Copy link

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Add authentication middleware to protect internal metrics.

The SummaryRoutes is mounted without authentication, making internal system metrics (monitor counts, incident counts) publicly accessible. All other non-public routes in this file use verifyJWT middleware for protection (see lines 35-45).

Exposing these metrics without authentication is a security risk as it reveals:

  • The number and status of monitored services
  • Infrastructure details (hardware monitor counts)
  • Active incident counts

This information could be valuable to attackers for reconnaissance.

Apply this diff to add authentication:

-	app.use(SummaryRoutes); //used for Homepage widget
+	app.use("/api/v1", verifyJWT, SummaryRoutes); //used for Homepage widget

Note: You'll also need to update the route definition in summaryRoute.js line 7 from /api/v1/summary to just /summary since the /api/v1 prefix will be handled by the mount point.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In server/src/config/routes.js around line 46, SummaryRoutes is mounted without
authentication; update the mount to use the existing verifyJWT middleware (same
pattern as lines 35-45) so SummaryRoutes is protected (e.g., app.use('/api/v1',
verifyJWT, SummaryRoutes) or app.use(verifyJWT, SummaryRoutes) consistent with
surrounding mounts). Also update server/src/routes/summaryRoute.js at line 7 to
change the route path from '/api/v1/summary' to '/summary' so it relies on the
mount prefix for '/api/v1' and does not duplicate the prefix.

};
21 changes: 21 additions & 0 deletions server/src/routes/v1/summaryRoute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import express from "express";
import Monitor from "../../db/v1/models/Monitor.js";
import Incident from "../../db/v1/models/Incident.js";

const router = express.Router();

router.get("/api/v1/summary", async (req, res) => {
Comment on lines +5 to +7
Copy link

@llamapreview llamapreview bot Dec 9, 2025

Choose a reason for hiding this comment

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

P2 | Confidence: Medium

The route definition contains the full /api/v1/summary path within the router. In Express.js, routers should define relative paths, and the full path is composed when mounting the router via app.use(). This creates path duplication and confusion about the actual endpoint location. The current pattern in the codebase (as seen in other route files) is to define relative paths in routers (e.g., router.get("/", ...)) and mount them with full paths in routes.js. The current approach will cause issues when combined with the improper mounting in routes.js.

Suggested change
const router = express.Router();
router.get("/api/v1/summary", async (req, res) => {
router.get("/", async (req, res) => { // Changed from "/api/v1/summary" to "/"

Evidence: path:server/src/routes/v1/summaryRoute.js, method:get

Copy link

@llamapreview llamapreview bot Dec 9, 2025

Choose a reason for hiding this comment

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

P2 | Confidence: Medium

Speculative: The endpoint lacks comprehensive error handling and validation. While basic error handling exists, there's no validation of query parameters (if any are added later), no input sanitization, and no handling of edge cases like database connection failures beyond the general catch block. The API also doesn't document expected behavior or error responses. While the current implementation is minimal, establishing proper error patterns early prevents technical debt as the endpoint evolves.

try {
const [uptime, infrastructure, incidents] = await Promise.all([
Monitor.countDocuments({ status: true, type: { $ne: "hardware" } }),
Monitor.countDocuments({ status: true, type: "hardware" }),
Incident.countDocuments({ status: true })
]);
Comment on lines +9 to +13
Copy link

@llamapreview llamapreview bot Dec 9, 2025

Choose a reason for hiding this comment

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

P1 | Confidence: High

The incident query uses { status: true } which assumes incidents have a boolean status field. Based on the issue requirement for "open incidents," and common incident tracking patterns, this is likely incorrect. Most incident tracking systems use statuses like "open", "resolved", "investigating" etc., not booleans. This will either return 0 incidents (if the field is string-based) or incorrectly count resolved incidents (if boolean-based). This breaks the core business logic requirement of counting only "open" incidents as specified in the issue #2792.

Code Suggestion:

Incident.countDocuments({ endTime: null }) // Assuming open incidents have no endTime
// OR
Incident.countDocuments({ status: "open" }) // If using string status

Evidence: path:server/src/routes/v1/summaryRoute.js, search:Incident.countDocuments

res.json({ uptime, infrastructure, incidents });
} catch (error) {
console.error("Erreur /api/v1/summary :", error);
res.status(500).json({ error: "Erreur interne" });
}
Comment on lines +15 to +18
Copy link

@llamapreview llamapreview bot Dec 9, 2025

Choose a reason for hiding this comment

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

P2 | Confidence: High

The error message is in French ("Erreur interne") while the rest of the codebase appears to use English. This creates inconsistency in the codebase and provides a confusing error message to users/developers who expect English messages. Consistent language usage is important for maintainability and developer experience.

Suggested change
} catch (error) {
console.error("Erreur /api/v1/summary :", error);
res.status(500).json({ error: "Erreur interne" });
}
console.error("Error in /api/v1/summary:", error);
res.status(500).json({ error: "Internal server error" });

Evidence: path:server/src/routes/v1/summaryRoute.js, search:Erreur

});
Comment on lines +7 to +19
Copy link

@coderabbitai coderabbitai bot Dec 9, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Add authentication middleware and verify field semantics.

This endpoint has several critical issues:

  1. Authentication missing: The endpoint will be mounted without authentication (see routes.js line 46), exposing internal system metrics publicly. All other non-public routes use verifyJWT middleware for protection.

  2. Field semantics unclear: The query uses status: true, but the PR objectives state "services that are currently up". Verify that status: true actually means "up/active" in your Monitor and Incident schemas, not just "enabled". If status means "enabled" rather than "currently up", this query will return incorrect counts.

  3. French error messages: Lines 16-17 contain French text ("Erreur /api/v1/summary", "Erreur interne"), which violates English-first conventions and the i18n requirements mentioned in the PR checklist.

Apply this diff to fix the error messages:

   } catch (error) {
-    console.error("Erreur /api/v1/summary :", error);
-    res.status(500).json({ error: "Erreur interne" });
+    console.error("Error in /api/v1/summary:", error);
+    res.status(500).json({ error: "Internal server error" });
   }

For authentication, see the comment on routes.js line 46. For field semantics, run the following verification:

#!/bin/bash
# Verify the schema definition for Monitor and Incident models to confirm field semantics

echo "=== Checking Monitor model schema ==="
rg -A 10 "status.*:" server/src/db/v1/models/Monitor.js

echo "=== Checking Incident model schema ==="
rg -A 10 "status.*:" server/src/db/v1/models/Incident.js

echo "=== Checking if 'isActive' or similar field exists ==="
rg -n "isActive|isUp|active" server/src/db/v1/models/
🤖 Prompt for AI Agents 
In server/src/routes/v1/summaryRoute.js around lines 7-19, add the verifyJWT
middleware to the route so it requires authentication, replace the console.error
French messages with English (e.g., "Error /api/v1/summary" and "Internal server
error") and use the central logger instead of console where available; then
verify the Monitor and Incident model semantics (run the provided rg checks) and
if the models use a different field for current up-state (e.g., isUp, isActive)
change the query predicates from status: true to the correct field (or combine
checks like { status: true, isUp: true } if both are required) so the counts
reflect services currently up rather than merely enabled.


export default router;