From f5d266ddb0d34f3d80d1bbdc04f902e744349a8f Mon Sep 17 00:00:00 2001 From: Alban Mouton Date: Tue, 29 Sep 2026 10:29:11 +0200 Subject: [PATCH] fix(nhis): do not record a last-logged date on a read-only storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The NHI token exchange called updateLogged unconditionally, while the password path guards it with `if (!storage.readonly)` in confirmLog. Under file storage that made the exchange fail outright: FileStorage.updateLogged throws `Method not implemented.` SYNCHRONOUSLY — it is not async — so the `.catch()` on the returned promise never attached and the throw propagated as a 500. The effect was that an NHI could not obtain a session at all under file storage, even though everything else on that path works: getUser is a read FileStorage implements, and it carries an `nhi` field through verbatim. This was the only thing standing between a file-storage deployment and a working exchange. Fixed by applying the same readonly guard the login path already uses, so the two call sites agree. Two unit tests pin it: that the storage declares itself readonly (the flag callers must branch on), and that updateLogged throws synchronously — the second is what makes a `.catch()` insufficient, so if it ever becomes an async rejection the test fails and the guard can be removed. Found while giving the data-fair/agents dev stack fixture NHIs so its autonomous agents could complete a real token exchange in dev rather than only in staging. Co-Authored-By: Claude Opus 5 (1M context) --- api/src/auth/router.ts | 8 +++++++- tests/features/file-storage.unit.spec.ts | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/api/src/auth/router.ts b/api/src/auth/router.ts index 07e67683..633ec620 100644 --- a/api/src/auth/router.ts +++ b/api/src/auth/router.ts @@ -345,7 +345,13 @@ router.post('/nhi-token', async (req, res) => { // setSessionCookies only stamps boundIp for adminMode, which an NHI never has, so no conflict if (user.nhi.ipBinding) payload.boundIp = clientIp const token = await setSessionCookies(req, res, reqSitePath(req), payload, 'nhi-session', userOrg, { skipExchangeToken: true, exp }) - storages.globalStorage.updateLogged(user.id).catch((err: any) => internalError('nhi-update-logged', 'error while updating logged date', err)) + // Guarded on `readonly`, like confirmLog on the password path: a read-only storage cannot record a + // last-logged date, and FileStorage's updateLogged throws SYNCHRONOUSLY — it is not async — so the + // .catch() below never attaches and the rejection became a 500 that failed the whole exchange. + // Without the guard an NHI could not obtain a session at all under file storage. + if (!storages.globalStorage.readonly) { + storages.globalStorage.updateLogged(user.id).catch((err: any) => internalError('nhi-update-logged', 'error while updating logged date', err)) + } eventsLog.info('sd.auth.nhi.ok', `an NHI session was created for ${user.id}`, logContext) res.set('Cache-Control', 'no-store') res.send({ access_token: token, token_type: 'Bearer', expires_in: exp - nowSec }) diff --git a/tests/features/file-storage.unit.spec.ts b/tests/features/file-storage.unit.spec.ts index 854dcb59..ce38dc5d 100644 --- a/tests/features/file-storage.unit.spec.ts +++ b/tests/features/file-storage.unit.spec.ts @@ -53,4 +53,18 @@ test.describe('file storage interface', () => { assert.equal(ntag.department, 'dep1') assert.equal(ntag.departmentName, 'Dep 1') }) + + test('declares itself readonly, which is what callers must branch on', () => { + // The NHI token exchange and the password login both record a last-logged date, which a + // read-only storage cannot do. Both must therefore branch on this flag rather than rely on + // catching a failure — see the next test for why catching does not work. + assert.equal(storage.readonly, true) + }) + + test('updateLogged throws synchronously, so a .catch() cannot absorb it', () => { + // This is the reason the NHI exchange needs an explicit `readonly` guard rather than a .catch(): + // the method is not async, so the throw happens at the call site before any handler attaches and + // propagates as a 500. If it ever becomes an async rejection this test fails, and the guard can go. + assert.throws(() => storage.updateLogged('anyone')) + }) })