Skip to content

Commit 699fe58

Browse files
hanapotskiclaude
andcommitted
fix(security): require auth + ownership on profile update (IDOR)
PUT /api/accounts/:userid had no auth middleware, so anyone could overwrite any user's profile (firstName, lastName, email, tenantId) by id -- including the email tied to a victim's login, which combined with the password-reset flow could be chained toward account takeover (security audit finding #2, Critical). Fix: - Add jwtSession.validateUser to the route so the endpoint requires a valid session (401 otherwise). - Enforce ownership in the controller: a user may only update their own profile (req.user.id === :userid), unless they hold an account- management admin role (admin / security_admin / global_admin); returns 403 otherwise. The only client caller (Profile.tsx self-edit) is unaffected: it edits the logged-in user's own id and the same-origin jwt cookie is sent automatically. Adds controller tests for owner-allowed, non-owner-blocked (IDOR), and admin-allowed cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 5c06bea commit 699fe58

3 files changed

Lines changed: 110 additions & 1 deletion

File tree

server/__test__/account.test.ts

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -654,4 +654,89 @@ describe("Account", () => {
654654
expect(removeMock).toHaveBeenCalledTimes(1);
655655
expect(res.sendStatus).toHaveBeenCalledWith(200);
656656
});
657+
658+
it("lets a user update their own profile", async () => {
659+
const res = mockResponse();
660+
const req = mockRequest({
661+
params: { userid: "123" },
662+
user: { id: 123, email: "user@test.com", sub: "data_entry" },
663+
body: {
664+
firstName: "New",
665+
lastName: "Name",
666+
email: "user@test.com",
667+
tenantId: "1",
668+
},
669+
});
670+
const next = mockNext();
671+
const updateUserProfileMock =
672+
accountService.updateUserProfile as jest.MockedFunction<
673+
typeof accountService.updateUserProfile
674+
>;
675+
updateUserProfileMock.mockResolvedValueOnce({
676+
isSuccess: true,
677+
code: "UPDATE_SUCCESS",
678+
message: "User profile successfully updated",
679+
} as any);
680+
681+
await accountController.updateUserProfile(req, res, next);
682+
expect(updateUserProfileMock).toHaveBeenCalledWith(
683+
"123",
684+
"New",
685+
"Name",
686+
"user@test.com",
687+
"1"
688+
);
689+
expect(res.status).toHaveBeenCalledWith(200);
690+
});
691+
692+
it("blocks a user from updating another user's profile (IDOR)", async () => {
693+
const res = mockResponse();
694+
const req = mockRequest({
695+
params: { userid: "999" },
696+
user: { id: 123, email: "user@test.com", sub: "data_entry" },
697+
body: {
698+
firstName: "Attacker",
699+
lastName: "Controlled",
700+
email: "victim-new@test.com",
701+
tenantId: "1",
702+
},
703+
});
704+
const next = mockNext();
705+
const updateUserProfileMock =
706+
accountService.updateUserProfile as jest.MockedFunction<
707+
typeof accountService.updateUserProfile
708+
>;
709+
710+
await accountController.updateUserProfile(req, res, next);
711+
expect(updateUserProfileMock).not.toHaveBeenCalled();
712+
expect(res.status).toHaveBeenCalledWith(403);
713+
});
714+
715+
it("lets an admin update another user's profile", async () => {
716+
const res = mockResponse();
717+
const req = mockRequest({
718+
params: { userid: "999" },
719+
user: { id: 123, email: "admin@test.com", sub: "admin,global_admin" },
720+
body: {
721+
firstName: "New",
722+
lastName: "Name",
723+
email: "someone@test.com",
724+
tenantId: "1",
725+
},
726+
});
727+
const next = mockNext();
728+
const updateUserProfileMock =
729+
accountService.updateUserProfile as jest.MockedFunction<
730+
typeof accountService.updateUserProfile
731+
>;
732+
updateUserProfileMock.mockResolvedValueOnce({
733+
isSuccess: true,
734+
code: "UPDATE_SUCCESS",
735+
message: "User profile successfully updated",
736+
} as any);
737+
738+
await accountController.updateUserProfile(req, res, next);
739+
expect(updateUserProfileMock).toHaveBeenCalledTimes(1);
740+
expect(res.status).toHaveBeenCalledWith(200);
741+
});
657742
});

server/app/controllers/account-controller.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,26 @@ const updateUserProfile: RequestHandler<
270270
never
271271
> = async (req, res) => {
272272
const userid = req.params.userid;
273+
274+
// Authorization: a user may only update their own profile, unless they hold
275+
// an account-management admin role. Prevents the IDOR where any authenticated
276+
// (or, previously, unauthenticated) caller could overwrite any user's profile
277+
// -- including the email tied to their login (security audit finding #2).
278+
const roles = new Set(
279+
(req.user?.sub || req.user?.role || "").split(",").filter(Boolean)
280+
);
281+
const isAccountAdmin =
282+
roles.has("admin") ||
283+
roles.has("security_admin") ||
284+
roles.has("global_admin");
285+
if (String(req.user?.id) !== String(userid) && !isAccountAdmin) {
286+
return res.status(403).json({
287+
isSuccess: false,
288+
code: "FORBIDDEN",
289+
message: "You are not authorized to update this profile.",
290+
});
291+
}
292+
273293
const { firstName, lastName, email, tenantId } = req.body;
274294
const response = await accountService.updateUserProfile(
275295
userid,

server/app/routes/account-router.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,11 @@ router.get("/logout", (req, res) => {
4545
res.sendStatus(200);
4646
});
4747

48-
router.put("/:userid", accountController.updateUserProfile);
48+
router.put(
49+
"/:userid",
50+
jwtSession.validateUser,
51+
accountController.updateUserProfile
52+
);
4953

5054
router.get("/:email", accountController.getByEmail);
5155

0 commit comments

Comments
 (0)