From 4932f28da51a2a5fb6ee8f7e1b94636a848404d3 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 19 May 2026 04:22:12 -0500 Subject: [PATCH] =?UTF-8?q?fix(billingtype):=20return=20404=20(not=20403)?= =?UTF-8?q?=20on=20cross-tenant=20access=20=E2=80=94=20close=20tenant-enum?= =?UTF-8?q?eration=20leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET/PATCH/DELETE on `/v1/billingtype/:id` returned: - 404 "Not found" when `:id` did not exist - 403 "Invalid Authorization Key" when the row existed but belonged to a different tenant A scoped (non-master) caller can iterate btId values and learn which billing-type ids are populated across the whole tenant table by status code. Same class of bug as `/v1/company`, fixed in #174 — applied here to billingtype's three single-resource handlers. The pattern propagates to every soft-deletable entity in the codebase (worker, customer, invoice, job, …). They'll get their own PRs in follow-up iterations — one entity per PR so each lands with focused tests and an isolated diff. Pinned in `tests/api/billingtype.test.js` as three controller-level unit tests (one per handler), driving the controller directly with stubbed `BillingType.findByPk` returning a row in tenant 99 while spying on `auth.isMaster` / `auth.getCompanyId` to make the caller appear scoped to tenant 7. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/billingtypecontroller.js | 15 ++++- tests/api/billingtype.test.js | 86 ++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/app/controllers/billingtypecontroller.js b/app/controllers/billingtypecontroller.js index 1d0c656..00dbc2e 100644 --- a/app/controllers/billingtypecontroller.js +++ b/app/controllers/billingtypecontroller.js @@ -91,8 +91,13 @@ exports.getById = async (req, res) => { const isMaster = await IsMaster(authKey); if (!isMaster) { const companyId = await GetCompanyId(authKey); + // Cross-tenant access is reported as 404, not 403 — otherwise + // a scoped caller can enumerate which BillingType ids are + // populated across the whole tenant table by status code: + // 403 = "exists but not yours", 404 = "doesn't exist". Same + // secure-404 pattern landed for /v1/company in #174. if (companyId === -1 || billingType.btCompId !== 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.", billingType }); @@ -168,8 +173,11 @@ exports.update = async (req, res) => { const isMaster = await IsMaster(authKey); if (!isMaster) { const companyId = await GetCompanyId(authKey); + // Secure-404 on PATCH for the same reason as GET — don't let + // a scoped caller distinguish "exists but not yours" from + // "doesn't exist". if (companyId === -1 || billingType.btCompId !== companyId) { - return res.status(403).json({ message: "Invalid Authorization Key." }); + return res.status(404).json({ message: "Not found." }); } } @@ -211,8 +219,9 @@ exports.remove = async (req, res) => { const isMaster = await IsMaster(authKey); if (!isMaster) { const companyId = await GetCompanyId(authKey); + // Secure-404 on DELETE for the same reason as GET / PATCH. if (companyId === -1 || billingType.btCompId !== companyId) { - return res.status(403).json({ message: "Invalid Authorization Key." }); + return res.status(404).json({ message: "Not found." }); } } diff --git a/tests/api/billingtype.test.js b/tests/api/billingtype.test.js index 4841249..81a1040 100644 --- a/tests/api/billingtype.test.js +++ b/tests/api/billingtype.test.js @@ -67,6 +67,92 @@ describe('BillingType route mounting', () => { }); }); +describe('BillingType tenant-enumeration defense (secure 404)', () => { + // Same approach as the company secure-404 unit tests: drive the + // controller directly with stubbed Model + spied auth helpers, so + // we don't have to wire every upstream middleware. The HTTP-level + // mock above makes findByPk resolve to null by default, which + // short-circuits to 404 before the cross-tenant branch is reached. + test('controller getById: existing-but-not-yours returns 404 to non-master', async () => { + const auth = require('../../app/middleware/auth.js'); + const controller = require('../../app/controllers/billingtypecontroller.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.BillingType.findByPk = vi.fn().mockResolvedValue({ + btId: 99, btCompId: 99, btArch: 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/billingtypecontroller.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.BillingType.findByPk = vi.fn().mockResolvedValue({ + btId: 99, btCompId: 99, btArch: false, update: vi.fn(), + }); + const req = { + get: (h) => (h === 'authKey' ? 'scoped-to-7' : undefined), + params: { id: 99 }, + body: { btName: '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(); + } + }); + + test('controller remove: existing-but-not-yours returns 404 to non-master', async () => { + const auth = require('../../app/middleware/auth.js'); + const controller = require('../../app/controllers/billingtypecontroller.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.BillingType.findByPk = vi.fn().mockResolvedValue({ + btId: 99, btCompId: 99, btArch: false, update: vi.fn(), + }); + 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.remove(req, res); + expect(captured.code).toBe(404); + expect(captured.body.message).toMatch(/not found/i); + } finally { + isMasterSpy.mockRestore(); + getCompanyIdSpy.mockRestore(); + } + }); +}); + describe('BillingType body validation', () => { test('POST rejects unknown field with 400', async () => { const res = await request(app)