Skip to content

Commit b08e9a3

Browse files
fix(users): admin GET /users/:id no longer leaks password hash and tokens (#3731)
* fix(users): admin GET /users/:id no longer leaks password hash and tokens `get` handler called `req.model.toJSON()` (raw Mongoose doc, no strip) while `list` correctly used `UserService.removeSensitive`. Aligns `get` with `list` by routing through `removeSensitive`. Adds an integration test asserting `password`, `resetPasswordToken`, `emailVerificationToken`, and `salt` are absent from the response. Closes #3723 * test(users): seed tokens before PII-leak assertion to make regression meaningful The previous assertion would pass vacuously when resetPasswordToken and emailVerificationToken are absent on the user. Seed both fields via updateById before the admin GET so the test proves the leak is actually blocked, not just that the fields were never present. Addresses: #3731 (comment)...
1 parent 0547013 commit b08e9a3

2 files changed

Lines changed: 41 additions & 1 deletion

File tree

modules/users/controllers/users.admin.controller.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ const list = async (req, res) => {
4141
*/
4242
const get = async (req, res) => {
4343
try {
44-
const user = req.model ? req.model.toJSON() : {};
44+
const user = req.model ? UserService.removeSensitive(req.model) : {};
4545
const memberships = await MembershipService.listByUser(user._id || user.id);
4646
user.memberships = memberships.map((m) => {
4747
const obj = m.toJSON ? m.toJSON() : { ...m };

modules/users/tests/user.admin.integration.tests.js

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,46 @@ describe('User admin integration tests:', () => {
225225
}
226226
});
227227

228+
test('should not expose sensitive fields (password, tokens) in admin GET /users/:id', async () => {
229+
try {
230+
userEdited = await signupAndPromoteAdmin(agent, { ..._userEdited, roles: ['user', 'admin'] });
231+
} catch (err) {
232+
console.log(err);
233+
expect(err).toBeFalsy();
234+
}
235+
236+
// Seed token fields so the regression test is meaningful — without this,
237+
// the assertions pass vacuously because the fields are simply absent.
238+
try {
239+
await UserService.updateById(userEdited._id, {
240+
resetPasswordToken: 'test-reset-token',
241+
emailVerificationToken: 'test-verification-token',
242+
});
243+
} catch (err) {
244+
console.log(err);
245+
expect(err).toBeFalsy();
246+
}
247+
248+
try {
249+
const result = await agent.get(`/api/admin/users/${userEdited._id}`).expect(200);
250+
expect(result.body.data).toBeInstanceOf(Object);
251+
expect(result.body.data.password).toBeUndefined();
252+
expect(result.body.data.resetPasswordToken).toBeUndefined();
253+
expect(result.body.data.emailVerificationToken).toBeUndefined();
254+
expect(result.body.data.salt).toBeUndefined();
255+
} catch (err) {
256+
console.log(err);
257+
expect(err).toBeFalsy();
258+
}
259+
260+
try {
261+
await UserService.remove(userEdited);
262+
} catch (err) {
263+
console.log(err);
264+
expect(err).toBeFalsy();
265+
}
266+
});
267+
228268
test('should be able to update a single user details if admin', async () => {
229269
try {
230270
userEdited = await signupAndPromoteAdmin(agent, { ..._userEdited, roles: ['user', 'admin'] });

0 commit comments

Comments
 (0)