diff --git a/app/controllers/companycontroller.js b/app/controllers/companycontroller.js index 536506c..2c8b10e 100644 --- a/app/controllers/companycontroller.js +++ b/app/controllers/companycontroller.js @@ -75,8 +75,15 @@ exports.getById = async (req, res) => { const isMaster = await IsMaster(authKey); if (!isMaster) { const companyId = await GetCompanyId(authKey); + // Cross-tenant access (caller's company doesn't match the + // looked-up company) is reported as 404, not 403. Returning + // 403 here would let a non-master caller enumerate which + // companies exist by probing IDs: 403 = "this company exists + // but isn't yours", 404 = "doesn't exist at all". Collapse + // both into 404 so the API doesn't leak the size of the + // tenant table to scoped keys. if (companyId === -1 || company.compId !== companyId) { - return res.status(403).json({ message: "Invalid Authorization Key." }); + return res.status(404).json({ message: "Not found." }); } } return res.status(200).json({ message: "Found.", company }); @@ -142,8 +149,11 @@ exports.update = async (req, res) => { const isMaster = await IsMaster(authKey); if (!isMaster) { const companyId = await GetCompanyId(authKey); + // Same secure-404 pattern as getById: don't let a non-master + // caller distinguish "this company exists but isn't yours" + // from "this id doesn't exist" by status code. if (companyId === -1 || company.compId !== companyId) { - return res.status(403).json({ message: "Invalid Authorization Key." }); + return res.status(404).json({ message: "Not found." }); } } diff --git a/tests/api/company.test.js b/tests/api/company.test.js index 24a209b..1c80435 100644 --- a/tests/api/company.test.js +++ b/tests/api/company.test.js @@ -76,6 +76,84 @@ describe('Company route mounting', () => { }); }); +describe('Company tenant-enumeration defense (secure 404)', () => { + // Unit-level: exercise the controller's getById/update directly with + // a stub `res` and an explicit `Company` model whose findByPk + // returns the "exists but not yours" shape. Sidesteps the HTTP + // pipeline so we don't have to wire every upstream middleware + // through testing seams — the assertion is just about the response + // code the controller chose for the cross-tenant branch. + // + // Why test at this level + // The HTTP-level mock used by the other tests in this file makes + // `Company.findByPk` resolve to `null` by default, which the + // controller short-circuits to a 404 BEFORE the cross-tenant + // branch is reached. Driving findByPk to return a populated row + // from the HTTP layer requires reaching deep into vitest module + // state in a way that fights vi.mock's hoisting; the controller- + // level test below pins the same behavioral guarantee much more + // cleanly. + + test('controller getById: existing-but-not-yours returns 404 to non-master', async () => { + // Use the exposed _internals seam (companycontroller.js:198). + // The controller's getById path uses IsMaster + GetCompanyId + // captured as module-locals; we can't rebind those across the + // ESM boundary, so we drive them indirectly by spying on + // auth.js's own exports — the helpers the captured locals point + // to. + const auth = require('../../app/middleware/auth.js'); + const controller = require('../../app/controllers/companycontroller.js'); + const isMasterSpy = vi.spyOn(auth, 'isMaster').mockResolvedValue(false); + const getCompanyIdSpy = vi.spyOn(auth, 'getCompanyId').mockResolvedValue(7); + try { + const db = require('../../app/config/db.config.js'); + db.Company.findByPk = vi.fn().mockResolvedValue({ compId: 99, compArch: false }); + + const req = { get: (h) => (h === 'authKey' ? 'scoped-to-7' : undefined), params: { id: 99 } }; + let captured = null; + const res = { + status(code) { this._code = code; return this; }, + json(body) { captured = { code: this._code, body }; return this; }, + }; + await controller.getById(req, res); + expect(captured.code).toBe(404); + expect(captured.body.message).toMatch(/not found/i); + } finally { + isMasterSpy.mockRestore(); + getCompanyIdSpy.mockRestore(); + } + }); + + test('controller update: existing-but-not-yours returns 404 to non-master', async () => { + const auth = require('../../app/middleware/auth.js'); + const controller = require('../../app/controllers/companycontroller.js'); + const isMasterSpy = vi.spyOn(auth, 'isMaster').mockResolvedValue(false); + const getCompanyIdSpy = vi.spyOn(auth, 'getCompanyId').mockResolvedValue(7); + try { + const db = await import('../../app/config/db.config.js'); + db.Company.findByPk = vi.fn().mockResolvedValue({ + compId: 99, compArch: false, update: vi.fn(), + }); + const req = { + get: (h) => (h === 'authKey' ? 'scoped-to-7' : undefined), + params: { id: 99 }, + body: { compName: 'X' }, + }; + let captured = null; + const res = { + status(code) { this._code = code; return this; }, + json(body) { captured = { code: this._code, body }; return this; }, + }; + await controller.update(req, res); + expect(captured.code).toBe(404); + expect(captured.body.message).toMatch(/not found/i); + } finally { + isMasterSpy.mockRestore(); + getCompanyIdSpy.mockRestore(); + } + }); +}); + describe('Company body validation', () => { test('POST rejects unknown field with 400', async () => { const res = await request(app)