From 01d338c2d7310ca0f2509271ab91f3ded7071ee2 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 19 May 2026 03:42:30 -0500 Subject: [PATCH] =?UTF-8?q?fix(company):=20return=20404=20(not=20403)=20on?= =?UTF-8?q?=20cross-tenant=20read/update=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 /v1/company/:id and PATCH /v1/company/:id used to respond: - 404 "Not found" when the company id didn't exist - 403 "Invalid Authorization Key" when the company existed but belonged to another tenant That distinction is observable by a scoped (non-master) caller, who could iterate through compId values and learn the size + sparsity of the entire tenant table — even though they're authorized to see only their own row. Switch the cross-tenant branch to 404 with the same "Not found" body the absent-id branch uses, so the two cases are indistinguishable. Master-key callers continue to see every company (no behavior change for them); scoped callers continue to see their own row at 200. Pinned in tests/api/company.test.js as two controller-level unit tests (one for getById, one for update). Tests drive the controller directly with stubbed `Company.findByPk` returning a company in tenant 99 while spying on auth.isMaster / auth.getCompanyId to make the caller appear scoped to tenant 7 — the cross-tenant mismatch that previously produced 403. Tenant-list enumeration via timing (a 404-after-DB-lookup is slower than a 404 with no lookup) is a separate, smaller leak left for a follow-up: reordering auth-check before findByPk closes it but changes more code than this one-concern PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/companycontroller.js | 14 ++++- tests/api/company.test.js | 78 ++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) 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)