diff --git a/app/controllers/customercontroller.js b/app/controllers/customercontroller.js index 37e32e6..84fa0be 100644 --- a/app/controllers/customercontroller.js +++ b/app/controllers/customercontroller.js @@ -539,6 +539,23 @@ exports.search = async (req, res) => { async function findAndRespond(customerId, res) { try { const customer = await Customer.findByPk(customerId); + if (!customer) { + // The master-key branch in getCustomerById calls + // findAndRespond without first verifying the row exists, + // so a non-existent customerId used to land here and + // return `200 { customers: null }` — a confusing + // success envelope wrapping an empty body. The scoped + // (non-master) branch already short-circuits to 403 on + // `GetCustomerCompanyId(id) === -1`, so it never hit this + // fallthrough. Make the master path return a proper 404 + // so both paths agree that "no such customer id" is + // a not-found, not a success. + // + // The `defaultScope` on the Customer model filters archived + // rows, so findByPk also returns null for soft-deleted + // customers — soft-deletes correctly surface as 404 too. + return res.status(404).json({ message: "Not found." }); + } return res.status(200).json({ message: "Successfully retrieved the customer with CustomerId " + customerId, customers: customer, diff --git a/tests/api/customer.test.js b/tests/api/customer.test.js index f417886..819679c 100644 --- a/tests/api/customer.test.js +++ b/tests/api/customer.test.js @@ -101,4 +101,39 @@ describe('GET /v1/customer/:id', () => { // 500 would indicate the error escaped its try/catch. expect(res.status).toBe(403); }); + + test('master key + non-existent customer id returns 404, not 200+null', async () => { + // Drive the master-key branch of getCustomerById directly so we + // can pin findAndRespond's null-row path. Before the fix, a + // null-returning findByPk emitted `200 { customers: null }` — + // a success envelope wrapping an empty body that any client + // checking `body.customers.custId` would crash on. + // + // Use the P5-M _setDbForTesting seam on auth.js: stub + // ApiMaster.findOne to return a row so real `IsMaster(authKey)` + // resolves true. vi.spyOn(auth.isMaster) doesn't intercept the + // captured `const IsMaster = auth.isMaster` reference in the + // controller — the seam is the documented hook. + const auth = require('../../app/middleware/auth.js'); + const customer = require('../../app/controllers/customercontroller.js'); + auth._setDbForTesting({ + ApiMaster: { findOne: vi.fn().mockResolvedValue({ amId: 1 }) }, + ApiKey: { findOne: vi.fn() }, + }); + try { + const db = require('../../app/config/db.config.js'); + db.Customer.findByPk = vi.fn().mockResolvedValue(null); + const req = { get: (h) => (h === 'authKey' ? 'master' : undefined), params: { id: 999999 } }; + let captured = null; + const res = { + status(code) { this._code = code; return this; }, + json(body) { captured = { code: this._code, body }; return this; }, + }; + await customer.getCustomerById(req, res); + expect(captured.code).toBe(404); + expect(captured.body.message).toMatch(/not found/i); + } finally { + auth._setDbForTesting(null); + } + }); });