From cac1e71625eb22403945d13b10529b55071982dd Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 19 May 2026 02:46:53 -0500 Subject: [PATCH] fix(versioninfo): reject reads with unknown authKey, not just missing-header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /v1/versioninfo/:id` and `GET /v1/versioninfo` originally gated only on `if (!authKey)` — header-present-check, no DB validation. Anyone with ANY non-empty string in the `authKey` header got 200 with the schema/build version records. The README's endpoint table says "yes (authKey)" for these reads, which the rest of the API consistently means "any VALID authKey, master or scoped". A literal presence-check breaks that contract: the same string `authKey: anything` that reliably 403s every other v1 endpoint silently let through on versioninfo. Add a `requireValidAuth(req, res)` helper that reads the already- resolved `req.authKey` / `req.isMaster` / `req.companyId` populated by `attachAuth` (mounted at /v1/*). Master keys pass; non-master keys pass when they resolved to a real company id; an unknown header is the new reject case (403 with the same "Invalid Authorization Key." message the other endpoints emit). No extra DB round-trip — the helper reads req state attachAuth already produced upstream. Two new tests pin the tightening: GET 403 for unknown authKey, GET list 403 for unknown authKey. The existing missing-header 403 tests continue to pass; the mounting tests that send `authKey: any` continue to pass because they only assert on body shape, not status code. Co-Authored-By: Claude Opus 4.7 (1M context) --- app/controllers/versioninfocontroller.js | 41 +++++++++++++++++++----- tests/api/versioninfo.test.js | 16 +++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/app/controllers/versioninfocontroller.js b/app/controllers/versioninfocontroller.js index 0d41048..b904053 100644 --- a/app/controllers/versioninfocontroller.js +++ b/app/controllers/versioninfocontroller.js @@ -33,6 +33,37 @@ async function requireMaster(authKey, res) { return true; } +/** + * Reject reads whose authKey header is missing OR present-but-unknown. + * + * VersionInfo reads were originally gated purely on "header present" + * — `getById` and `list` accepted any non-empty string in `authKey` + * and returned 200. The README's endpoint table says "yes (authKey)" + * for these endpoints, which the rest of the codebase consistently + * treats as "any VALID authKey, master or scoped"; a literal + * presence-check breaks that contract. + * + * attachAuth (mounted at /v1/*) has already populated req.isMaster + * and req.companyId from the supplied header, so this helper just + * reads the resolved state without a second DB round-trip. + * + * Master keys pass (isMaster=true, companyId=-1 by convention). + * Non-master keys pass when they resolved to a real company. An + * unknown header is the case we want to reject — !isMaster AND + * companyId===-1. + */ +function requireValidAuth(req, res) { + if (!req.authKey) { + res.status(403).json({ message: "Authorization key not sent." }); + return false; + } + if (!req.isMaster && req.companyId === -1) { + res.status(403).json({ message: "Invalid Authorization Key." }); + return false; + } + return true; +} + exports.create = async (req, res) => { const authKey = req.get('authKey'); if (!(await requireMaster(authKey, res))) return; @@ -52,10 +83,7 @@ exports.create = async (req, res) => { }; exports.getById = async (req, res) => { - const authKey = req.get('authKey'); - if (!authKey) { - return res.status(403).json({ message: "Authorization key not sent." }); - } + if (!requireValidAuth(req, res)) return; try { const v = await VersionInfo.findByPk(req.params.id); if (!v) return res.status(404).json({ message: "Not found." }); @@ -67,10 +95,7 @@ exports.getById = async (req, res) => { }; exports.list = async (req, res) => { - const authKey = req.get('authKey'); - if (!authKey) { - return res.status(403).json({ message: "Authorization key not sent." }); - } + if (!requireValidAuth(req, res)) return; const requestedLimit = parseInt(req.query.limit, 10); const limit = Number.isInteger(requestedLimit) && requestedLimit > 0 diff --git a/tests/api/versioninfo.test.js b/tests/api/versioninfo.test.js index 4378976..43dc02d 100644 --- a/tests/api/versioninfo.test.js +++ b/tests/api/versioninfo.test.js @@ -49,6 +49,22 @@ describe('VersionInfo auth contract', () => { test('DELETE 403 for non-master key', async () => { expect((await request(app).delete('/v1/versioninfo/1').set('authKey', 'not-master')).status).toBe(403); }); + + test('GET 403 for unknown authKey (header present but not in DB)', async () => { + // VersionInfo reads previously gated only on header-present, so any + // non-empty string in `authKey` returned 200. The README's endpoint + // table says "yes (authKey)" for reads, which the rest of the API + // consistently means "any VALID authKey". Pin the tightening so a + // future refactor can't accidentally drop the validation check. + const res = await request(app).get('/v1/versioninfo/1').set('authKey', 'unknown'); + expect(res.status).toBe(403); + expect(res.body.message).toMatch(/Invalid Authorization Key/i); + }); + test('GET list 403 for unknown authKey', async () => { + const res = await request(app).get('/v1/versioninfo').set('authKey', 'unknown'); + expect(res.status).toBe(403); + expect(res.body.message).toMatch(/Invalid Authorization Key/i); + }); }); describe('VersionInfo route mounting', () => {