fix: use a public user object to prevent passwords and other secrets in user objects

Author: andypols

src/db/mongo/users.ts

@@ -17,7 +17,7 @@ export const getUsers = async function (query: any = {}) {
   }
   console.log(`Getting users for query= ${JSON.stringify(query)}`);
   const collection = await connect(collectionName);
-  return collection.find(query, { password: 0 }).toArray();
+  return collection.find(query).project({ password: 0 }).toArray();
 };
 
 export const deleteUser = async function (username: string) {

src/service/routes/users.js

@@ -2,6 +2,17 @@ const express = require('express');
 const router = new express.Router();
 const db = require('../../db');
 
+const toPublicUser = (user) => {
+  return {
+    username: user.username || '',
+    displayName: user.displayName || '',
+    email: user.email || '',
+    title: user.title || '',
+    gitAccount: user.gitAccount || '',
+    admin: user.admin || false,
+  }
+}
+
 router.get('/', async (req, res) => {
   const query = {};
 
@@ -17,16 +28,16 @@ router.get('/', async (req, res) => {
     query[k] = v;
   }
 
-  res.send(await db.getUsers(query));
+  const users = await db.getUsers(query);
+  res.send(users.map(toPublicUser));
 });
 
 router.get('/:id', async (req, res) => {
   const username = req.params.id.toLowerCase();
   console.log(`Retrieving details for user: ${username}`);
   const data = await db.findUser(username);
   const user = JSON.parse(JSON.stringify(data));
-  if (user && user.password) delete user.password;
-  res.send(user);
+  res.send(toPublicUser(user));
 });
 
 module.exports = router;

test/services/routes/users.test.js

@@ -0,0 +1,67 @@
+const chai = require('chai');
+const chaiHttp = require('chai-http');
+const sinon = require('sinon');
+const express = require('express');
+const usersRouter = require('../../../src/service/routes/users');
+const db = require('../../../src/db');
+
+const { expect } = chai;
+chai.use(chaiHttp);
+
+describe('Users API', function () {
+  let app;
+
+  before(function () {
+    app = express();
+    app.use(express.json());
+    app.use('/users', usersRouter);
+  });
+
+  beforeEach(function () {
+    sinon.stub(db, 'getUsers').resolves([
+      {
+        username: 'alice',
+        password: 'secret-hashed-password',
+        email: '[email protected]',
+        displayName: 'Alice Walker',
+      },
+    ]);
+    sinon
+      .stub(db, 'findUser')
+      .resolves({ username: 'bob', password: 'hidden', email: '[email protected]' });
+  });
+
+  afterEach(function () {
+    sinon.restore();
+  });
+
+  it('GET /users only serializes public data needed for ui, not user secrets like password', async function () {
+    const res = await chai.request(app).get('/users');
+    expect(res).to.have.status(200);
+    expect(res.body).to.deep.equal([
+      {
+        username: 'alice',
+        displayName: 'Alice Walker',
+        email: '[email protected]',
+        title: '',
+        gitAccount: '',
+        admin: false,
+      },
+    ]);
+  });
+
+  it('GET /users/:id does not serialize password', async function () {
+    const res = await chai.request(app).get('/users/bob');
+    expect(res).to.have.status(200);
+    console.log(`Response body: ${res.body}`);
+
+    expect(res.body).to.deep.equal({
+      username: 'bob',
+      displayName: '',
+      email: '[email protected]',
+      title: '',
+      gitAccount: '',
+      admin: false,
+    });
+  });
+});

test/testDb.test.js

@@ -241,7 +241,7 @@ describe('Database clients', async () => {
       TEST_USER.admin,
     );
 
-    // has less properties to prove that records are merged
+    // has fewer properties to prove that records are merged
     const updateToApply = {
       username: TEST_USER.username,
       gitAccount: 'updatedGitAccount',