From d9cf277dd77195283b195dbf7b23bfd04a4bc98b Mon Sep 17 00:00:00 2001
From: "Aaron K. Clark" <akclark@thenetwerk.net>
Date: Tue, 19 May 2026 02:52:40 -0500
Subject: [PATCH] chore(auth): remove dead try/catch around isMaster +
 getCompanyId in attachAuth
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`isMaster` and `getCompanyId` both wrap their DB query in a
try/catch INTERNALLY (see auth.js:97-109 and 111-125): on any
Sequelize / pg error they `log.error` and return `false` or `-1`
respectively. The errors don't propagate.

So the outer `try { req.isMaster = await isMaster(authKey) }
catch { return res.status(500) }` in attachAuth had nothing to
catch — the awaited promise never rejects. Same for the
getCompanyId wrapper below it. Both `500` paths were unreachable
dead code that misled readers into thinking attachAuth
distinguished "auth failed" from "DB down".

Strip the dead try/catch. Add a comment explaining the
swallow-into-sentinel design so the next reader doesn't
re-introduce the outer catches, and call out the real fix path
if anyone DOES want DB-down to surface as 500 (push the throw
back up from isMaster / getCompanyId, then layer a catch here
that's actually reachable). Behaviour unchanged; test suite
unchanged at 614 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 app/middleware/auth.js | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/app/middleware/auth.js b/app/middleware/auth.js
index 0770deb..5845490 100644
--- a/app/middleware/auth.js
+++ b/app/middleware/auth.js
@@ -277,24 +277,27 @@ async function attachAuth(req, res, next) {
     req.isMaster = false;
     req.companyId = -1;
     if (!authKey) return next();
-    try {
-        req.isMaster = await isMaster(authKey);
-    } catch (error) {
-        log.error({ err: error }, 'attachAuth: isMaster failed');
-        return res.status(500).json({ message: 'Error!' });
-    }
+    // No try/catch here even though these are DB-touching calls:
+    // both `isMaster` and `getCompanyId` already wrap their query
+    // in try/catch internally and swallow infrastructure errors
+    // into a `false` / `-1` sentinel (with a `log.error`). So a
+    // DB outage surfaces as "auth failed" (403 downstream via
+    // requireAuth or per-controller checks), not "server error"
+    // — and the outer wrappers we had here were unreachable
+    // dead code that misled readers into thinking attachAuth
+    // distinguished the two. If the silent-swallow turns out to
+    // be the wrong behaviour for operators (DB-down looking like
+    // auth-fail), the right fix is to surface the throw from
+    // isMaster / getCompanyId themselves, not to layer a catch
+    // here that the inner function prevents from firing.
+    req.isMaster = await isMaster(authKey);
     if (req.isMaster) {
         // Master keys aren't scoped to a single company. Leave
         // companyId at -1; handlers needing a target scope read
         // it from req.params / req.body / req.query.
         return next();
     }
-    try {
-        req.companyId = await getCompanyId(authKey);
-    } catch (error) {
-        log.error({ err: error }, 'attachAuth: getCompanyId failed');
-        return res.status(500).json({ message: 'Error!' });
-    }
+    req.companyId = await getCompanyId(authKey);
     return next();
 }