Second-order IDOR in benefits handler persists after A7 fix

[cgdimarucut]

Summary

The A7 tutorial documents a missing access control vulnerability on the /benefits endpoints and provides a fix (applying the isAdmin middleware). However, even after applying the documented fix, the updateBenefits handler contains a second authorization gap: it accepts any userId from the POST body without validating that the requesting admin is not modifying their own benefits or another admin's benefits.

Vulnerable Code

File: app/routes/benefits.js, lines 27-45

this.updateBenefits = (req, res) => {
    const visitorId = req.body.visitorId;
    const visitorStartDate = req.body.visitorStartDate;
    // No check that visitorId != req.session.userId
    // No check that target user is not an admin
    benefitsDAO.updateBenefits(visitorId, visitorStartDate, ...);
};

Steps to Reproduce

1. Apply the documented A7 fix (uncomment isAdmin middleware on benefits routes)
2. Log in as an admin user
3. Send POST /benefits with { visitorId: <admin_user_id>, visitorStartDate: "2024-01-01" }
4. The admin successfully modifies their own benefit start date

Impact

This is a segregation of duties violation. The A7 fix prevents non-admin users from reaching the endpoint, but does not prevent:

• An admin from modifying their own benefits (self-approval)
• An admin from modifying another admin's benefits

In a real retirement/benefits management system, this would violate the principle that a person should not be able to approve their own benefits.

Suggested Fix

Add authorization validation in updateBenefits:

// Prevent admins from modifying their own benefits
if (visitorId === req.session.userId) {
    return res.render("benefits", { updateError: "Cannot modify your own benefits" });
}
// Prevent modification of other admin accounts
userDAO.getUserById(visitorId, (err, targetUser) => {
    if (targetUser && targetUser.isAdmin) {
        return res.render("benefits", { updateError: "Cannot modify admin benefits" });
    }
    // proceed with update
});