Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 44 additions & 6 deletions middlewares/validators/userStatus.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ const Joi = require("joi");
const { userState, CANCEL_OOO } = require("../../constants/userStatus");
const threeDaysInMilliseconds = 172800000;

const cancelOooSchema = Joi.object()
.keys({
cancelOoo: Joi.boolean().valid(true).required(),
})
.unknown(false);

const validateUserStatusData = async (todaysTime, req, res, next) => {
const validUserStates = [userState.OOO, userState.ONBOARDING];

Expand Down Expand Up @@ -58,12 +64,6 @@ const validateUserStatusData = async (todaysTime, req, res, next) => {
}),
});

const cancelOooSchema = Joi.object()
.keys({
cancelOoo: Joi.boolean().valid(true).required(),
})
.unknown(false);

let schema;
try {
if (Object.keys(req.body).includes(CANCEL_OOO)) {
Expand All @@ -86,6 +86,43 @@ const validateUserStatus = (req, res, next) => {
validateUserStatusData(todaysTime, req, res, next);
};

const validateUserStatusSelf = async (req, res, next) => {
const today = new Date();
today.setUTCHours(0, 0, 0, 0);
const todaysTime = today.getTime();

const selfStatusSchema = Joi.object({
currentStatus: Joi.object().keys({
state: Joi.forbidden().error(new Error("Updating 'state' is not allowed via this endpoint.")),
until: Joi.forbidden().error(new Error("Updating 'until' is not allowed via this endpoint.")),
updatedAt: Joi.number().required(),
from: Joi.number()
.min(todaysTime)
.required()
.error(new Error(`The 'from' field must have a value that is either today or a date that follows today.`)),
message: Joi.string().allow("").optional(),
}),
Comment on lines +94 to +104
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's locate and read the userStatus.js file
find . -name "userStatus.js" -type f

Repository: RealDevSquad/website-backend

Length of output: 280


🏁 Script executed:

# Read the full userStatus.js file to see the context
cat -n middlewares/validators/userStatus.js

Repository: RealDevSquad/website-backend

Length of output: 6836


🏁 Script executed:

# Search for references to "long OOO" or similar patterns in the file
rg -i "long.*ooo|ooo.*long|message.*required" middlewares/validators/userStatus.js -A 3 -B 3

Repository: RealDevSquad/website-backend

Length of output: 54


🏁 Script executed:

# Look for controller files that use this validator
rg -l "userStatus" --type js -g "*/controller*" -g "*/controllers*"

Repository: RealDevSquad/website-backend

Length of output: 220


🏁 Script executed:

# Read the userStatus controller
cat -n ./controllers/userStatus.js | head -200

Repository: RealDevSquad/website-backend

Length of output: 8153


🏁 Script executed:

# Search for where validateUserStatusSelf is used
rg "validateUserStatusSelf" -A 5 -B 5

Repository: RealDevSquad/website-backend

Length of output: 2586


🏁 Script executed:

# Look for any message validation or enforcement in the controller or model
rg "message.*empty|empty.*message|allow.*message" -i --type js

Repository: RealDevSquad/website-backend

Length of output: 828


🏁 Script executed:

# Check if there's any logic that validates message based on state in controller/model
rg "long.*OOO|OOO.*long" -i --type js -A 3 -B 3

Repository: RealDevSquad/website-backend

Length of output: 54


🏁 Script executed:

# Read the integration test for self status
cat -n test/integration/userStatusSelfRestricted.test.js | head -200

Repository: RealDevSquad/website-backend

Length of output: 3818


🏁 Script executed:

# Read the unit test that mentions empty strings
rg "should set the message and until properties to empty strings" -A 10 -B 5 test/unit/utils/userStatus.test.js

Repository: RealDevSquad/website-backend

Length of output: 725


🏁 Script executed:

# Search for any validation of message in the model or utils
rg "message" test/integration/userStatusSelfRestricted.test.js -A 3 -B 3

Repository: RealDevSquad/website-backend

Length of output: 1447


Address message validation inconsistency between endpoints.

The selfStatusSchema (line 103) allows clearing the message with .allow(""), but the primary validation in validateUserStatusData (lines 37-59) enforces that OOO status lasting more than 3 days requires a message. A user can set long OOO with a required message via the main endpoint, then clear it via the /self endpoint, creating an inconsistency.

Either disallow empty strings in the self endpoint by changing line 103 to message: Joi.string().min(1).optional(), or enforce the message requirement in the controller after the user's stored status is checked.

🤖 Prompt for AI Agents
In `@middlewares/validators/userStatus.js` around lines 94 - 104, selfStatusSchema
currently allows clearing the message via `.allow("")`, which creates an
inconsistency with validateUserStatusData that requires a message for OOO >3
days; update the selfStatusSchema's currentStatus.message definition to
`Joi.string().min(1).optional()` (replace the `.allow("")`) to disallow empty
strings, ensuring the self endpoint enforces the same non-empty message rule as
validateUserStatusData (verify behavior in any controller logic that reads
selfStatusSchema and adjust tests if present).

monthlyHours: Joi.object().keys({
committed: Joi.number().required(),
updatedAt: Joi.number().required(),
}),
});

let schema;
try {
if (Object.keys(req.body).includes(CANCEL_OOO)) {
schema = cancelOooSchema;
} else {
schema = selfStatusSchema;
}
await schema.validateAsync(req.body);
next();
} catch (error) {
logger.error(`Error validating UserStatus ${error}`);
res.boom.badRequest(error);
}
};

const validateMassUpdate = async (req, res, next) => {
const schema = Joi.object()
.keys({
Expand Down Expand Up @@ -139,4 +176,5 @@ module.exports = {
validateUserStatus,
validateMassUpdate,
validateGetQueryParams,
validateUserStatusSelf,
};
3 changes: 3 additions & 0 deletions models/userStatus.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,9 @@ const updateUserStatus = async (userId, updatedStatusData) => {
const previousUntil = previousCurrentStatus.until;
let requestedNextState;
if (Object.keys(newStatusData).includes("currentStatus")) {
if (!newStatusData.currentStatus.state && previousCurrentStatus.state) {
newStatusData.currentStatus = { ...previousCurrentStatus, ...newStatusData.currentStatus };
}
requestedNextState = newStatusData.currentStatus?.state;
const newUserState = requestedNextState;
const isNewStateOoo = newUserState === userState.OOO;
Expand Down
3 changes: 2 additions & 1 deletion routes/userStatus.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
validateUserStatus,
validateMassUpdate,
validateGetQueryParams,
validateUserStatusSelf,
} = require("../middlewares/validators/userStatus");
const { authorizeAndAuthenticate } = require("../middlewares/authorizeUsersAndService");
const ROLES = require("../constants/roles");
Expand All @@ -24,7 +25,7 @@
router.get("/", validateGetQueryParams, getUserStatusControllers);
router.get("/self", authenticate, getUserStatus);
router.get("/:userId", getUserStatus);
router.patch("/self", authenticate, validateUserStatus, updateUserStatusController); // this route is being deprecated, please use /users/status/:userId PATCH endpoint instead.
router.patch("/self", authenticate, validateUserStatusSelf, updateUserStatusController); // this route is being deprecated, please use /users/status/:userId PATCH endpoint instead.

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 19 days ago

In general, the fix is to introduce rate limiting middleware so that requests to the sensitive route cannot be made arbitrarily fast. In Express, this is commonly done via express-rate-limit, defining a limiter with appropriate windowMs and max settings, and applying it either globally or to specific routes.

For this file, the least intrusive and clearest fix is to: (1) import express-rate-limit, (2) create a limiter instance (e.g., userStatusSelfLimiter) configured for authenticated user operations, and (3) insert that limiter as middleware on the /self PATCH route (line 28) before authenticate. This approach keeps existing behavior intact while adding protection: the route path, validators, and controllers remain unchanged, and only an extra middleware is added in the chain. Concretely, near the top of routes/userStatus.js, after the existing require statements, add const rateLimit = require("express-rate-limit"); and define a limiter, e.g.:

const userStatusSelfLimiter = rateLimit({
  windowMs: 15 * 60 * 1000,
  max: 100,
});

Then change line 28 so that it uses this limiter: router.patch("/self", userStatusSelfLimiter, authenticate, validateUserStatusSelf, updateUserStatusController);. This ensures that calls to this authenticated, state-changing route are rate-limited without altering any other route behavior.

Suggested changeset 2
routes/userStatus.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/routes/userStatus.js b/routes/userStatus.js
--- a/routes/userStatus.js
+++ b/routes/userStatus.js
@@ -21,11 +21,17 @@
 const { authorizeAndAuthenticate } = require("../middlewares/authorizeUsersAndService");
 const ROLES = require("../constants/roles");
 const { Services } = require("../constants/bot");
+const rateLimit = require("express-rate-limit");
 
+const userStatusSelfLimiter = rateLimit({
+  windowMs: 15 * 60 * 1000, // 15 minutes
+  max: 100, // limit each IP to 100 requests per windowMs
+});
+
 router.get("/", validateGetQueryParams, getUserStatusControllers);
 router.get("/self", authenticate, getUserStatus);
 router.get("/:userId", getUserStatus);
-router.patch("/self", authenticate, validateUserStatusSelf, updateUserStatusController); // this route is being deprecated, please use /users/status/:userId PATCH endpoint instead.
+router.patch("/self", userStatusSelfLimiter, authenticate, validateUserStatusSelf, updateUserStatusController); // this route is being deprecated, please use /users/status/:userId PATCH endpoint instead.
 router.patch("/update", authorizeAndAuthenticate([ROLES.SUPERUSER], [Services.CRON_JOB_HANDLER]), updateAllUserStatus);
 router.patch(
   "/batch",
EOF
@@ -21,11 +21,17 @@
const { authorizeAndAuthenticate } = require("../middlewares/authorizeUsersAndService");
const ROLES = require("../constants/roles");
const { Services } = require("../constants/bot");
const rateLimit = require("express-rate-limit");

const userStatusSelfLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 100, // limit each IP to 100 requests per windowMs
});

router.get("/", validateGetQueryParams, getUserStatusControllers);
router.get("/self", authenticate, getUserStatus);
router.get("/:userId", getUserStatus);
router.patch("/self", authenticate, validateUserStatusSelf, updateUserStatusController); // this route is being deprecated, please use /users/status/:userId PATCH endpoint instead.
router.patch("/self", userStatusSelfLimiter, authenticate, validateUserStatusSelf, updateUserStatusController); // this route is being deprecated, please use /users/status/:userId PATCH endpoint instead.
router.patch("/update", authorizeAndAuthenticate([ROLES.SUPERUSER], [Services.CRON_JOB_HANDLER]), updateAllUserStatus);
router.patch(
"/batch",
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -42,7 +42,8 @@
     "passport-github2": "0.1.12",
     "passport-google-oauth20": "^2.0.0",
     "rate-limiter-flexible": "5.0.3",
-    "winston": "3.13.0"
+    "winston": "3.13.0",
+    "express-rate-limit": "^8.2.1"
   },
   "devDependencies": {
     "@types/chai": "4.3.16",
EOF
@@ -42,7 +42,8 @@
"passport-github2": "0.1.12",
"passport-google-oauth20": "^2.0.0",
"rate-limiter-flexible": "5.0.3",
"winston": "3.13.0"
"winston": "3.13.0",
"express-rate-limit": "^8.2.1"
},
"devDependencies": {
"@types/chai": "4.3.16",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
router.patch("/update", authorizeAndAuthenticate([ROLES.SUPERUSER], [Services.CRON_JOB_HANDLER]), updateAllUserStatus);
router.patch(
"/batch",
Expand Down
95 changes: 95 additions & 0 deletions test/integration/userStatusSelfRestricted.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
const chai = require("chai");
const { expect } = chai;
const chaiHttp = require("chai-http");
const app = require("../../server");
const authService = require("../../services/authService");
const addUser = require("../utils/addUser");
const cleanDb = require("../utils/cleanDb");
const { updateUserStatus } = require("../../models/userStatus");
const { generateUserStatusData } = require("../fixtures/userStatus/userStatus");
const config = require("config");
const cookieName = config.get("userToken.cookieName");
const firestore = require("../../utils/firestore");

chai.use(chaiHttp);

describe("Restricted PATCH /users/status/self", function () {
let jwt;
let userId = "";

beforeEach(async function () {
userId = await addUser();
jwt = authService.generateAuthToken({ userId });
const initialStatus = generateUserStatusData("OOO", Date.now(), Date.now(), Date.now() + 86400000);
await updateUserStatus(userId, initialStatus);
});

afterEach(async function () {
await cleanDb();
});

it("Should return 400 when trying to update 'state'", async function () {
const res = await chai
.request(app)
.patch("/users/status/self")
.set("cookie", `${cookieName}=${jwt}`)
.send({
currentStatus: {
state: "ACTIVE",
updatedAt: Date.now(),
from: Date.now(),
},
});

expect(res).to.have.status(400);
expect(res.body.error).to.equal("Bad Request");
expect(res.body.message).to.include("Updating 'state' is not allowed via this endpoint");
});

it("Should return 400 when trying to update 'until'", async function () {
const res = await chai
.request(app)
.patch("/users/status/self")
.set("cookie", `${cookieName}=${jwt}`)
.send({
currentStatus: {
until: Date.now() + 100000,
updatedAt: Date.now(),
from: Date.now(),
},
});

expect(res).to.have.status(400);
expect(res.body.error).to.equal("Bad Request");
expect(res.body.message).to.include("Updating 'until' is not allowed via this endpoint");
});

it("Should allow updating other fields (message) and preserve state/until", async function () {
// Current state is OOO. We update message.
const newMessage = "Updated message via restricted endpoint";
const now = Date.now();

const res = await chai
.request(app)
.patch("/users/status/self")
.set("cookie", `${cookieName}=${jwt}`)
.send({
currentStatus: {
message: newMessage,
updatedAt: now,
from: now,
},
});

expect(res).to.have.status(200);
expect(res.body.message).to.equal("User Status updated successfully.");

// Verify persistence
const doc = await firestore.collection("usersStatus").where("userId", "==", userId).get();
const data = doc.docs[0].data();

expect(data.currentStatus.state).to.equal("OOO");
expect(data.currentStatus.message).to.equal(newMessage);
expect(data.currentStatus.until).to.not.equal(undefined);
});
});
Loading