Logout leaves the session cookie in place when the revocation write fails

[lidonius1122]

Describe The Bug

Noticed this while testing e5f2f0d4 on beta-5.28.1 and mentioned it in an email thread with the security team — filing it here as well so it doesn't get lost. Measured against the e2e suite rather than a running install, hence the odd-looking environment fields below.

In logout(), revokeUserSessions() is awaited before the cookies are cleared. If it throws, the Set-Cookie line is never reached: the response is a 500 with no Set-Cookie header at all, and the hb-refresh cookie stays in the browser. POST /auth/session with it still returns 201 afterwards.

The UI catches the error and reloads regardless, so from the user's side: they click "log out", the page reloads, and they are back in the signed-in interface with no indication that anything went wrong.

Steps to reproduce (Nest e2e via app.inject, against 270ef02e):

1. Log in, keep the hb-refresh cookie.
2. Make the write fail. I used a read-only storage directory (EACCES when the temp file is opened) and, separately, a corrupted auth.json while the user cache was still warm.
3. POST /auth/logout → 500, no Set-Cookie.
4. POST /auth/session with the same cookie → 201, with a usable access token.

Control run with a writable directory: logout 201, and the same cookie then returns 401.

Neither failure is something an attacker can trigger, but both are plausible on a Pi — ext4 defaults to remounting read-only on error, and a full or dying SD card gets there. The corrupted-file variant also persists: the user "logs out", someone repairs auth.json later, and the cookie still restores the session because sessionVersion was never incremented.

Clearing the cookies even when the revocation fails would make logout fail closed. How you do that is your call — I've got a patch and a test ready if you want it, happy to open a PR or leave it with you.

Homebridge UI Version

beta-5.28.1 @ 270ef02

Homebridge Version

2.4.0

Node.js Version

24.19.0

Operating System

Other (specify in description)

Environment Info

Other (specify in description)

[bwp91]

Confirmed and fixed — both of your reproductions are now test cases on beta-5.28.1.

What changed

logout() was doing things in the wrong order. The cookie clearing and the in-memory plugin-ticket revocation now run first, unconditionally — neither needs the disk, and both were sitting behind a write that could fail. The sessionVersion write happens after, in its own try/catch.

When that write does fail, you now get the honest version of events in the log rather than a bare 500:

This browser has been signed out, but a session cookie captured elsewhere stays usable until the storage problem is fixed — log out again once it is.

Which is the limit you identified yourself: with the storage gone there is nothing to write the revocation to, so telling the user is all that can be done until the disk is back. Your corrupted-auth.json variant is why the message says "log out again" rather than pretending the cleanup will happen by itself.

The other thing your email surfaced

Your note that logout is now account-wide turned out to matter more than either of us gave it credit for at the time. The UI calls logout() from a few automatic places, so I went through them all:

• the 401 interceptor and the expired-token check fire with a token that is already invalid, so the server refuses to bump sessionVersion on those paths — they were fine already
• the inactivity timer is the one that fires with a valid token — so one idle tab forgotten on another machine would have ended your active sessions on every device, and it would have looked like the UI randomly logging you out

Nobody chose that logout, so it should not reach beyond the browser it happened in. The endpoint now takes a scope: the inactivity path asks for a browser-local one (this browser's cookies cleared, nothing revoked server-side), while a logout you actually click stays account-wide — that part is doing exactly what it was built to do.

On the PR offer

Thank you — and I'll be honest about why I took this one in-house: beta-5.28.1 gets rebased and rewritten regularly (twice today alone), so your branch would have gone stale underneath you before it could merge. Your report did the hard half, and the commit carries this issue number.

Both fixes will be in the next beta. Given the quality of your testing this week, I'd genuinely value a run of your scenarios against it once it's out — reopen this if anything survives.

[lidonius1122]

That inactivity path is a much better catch than the note I made — I'd only spotted that logout had become account-wide, not that one of the automatic callers still holds a valid token. Scoping it to the browser it happened in seems like the right call to me.

Makes sense on taking it in-house too, and glad the reproductions were useful as test cases.

Happy to run my scenarios against the next beta once it's out, and I'll report back either way.

[bwp91]

Confirmed and fixed — both of your reproductions are now test cases on beta-5.28.1.

What changed

logout() was doing things in the wrong order. The cookie clearing and the in-memory plugin-ticket revocation now run first, unconditionally — neither needs the disk, and both were sitting behind a write that could fail. The sessionVersion write happens after, in its own try/catch.

When that write does fail, you now get the honest version of events in the log rather than a bare 500:

This browser has been signed out, but a session cookie captured elsewhere stays usable until the storage problem is fixed — log out again once it is.

Which is the limit you identified yourself: with the storage gone there is nothing to write the revocation to, so telling the user is all that can be done until the disk is back. Your corrupted-auth.json variant is why the message says "log out again" rather than pretending the cleanup will happen by itself.

The other thing your email surfaced

Your note that logout is now account-wide turned out to matter more than either of us gave it credit for at the time. The UI calls logout() from a few automatic places, so I went through them all:

• the 401 interceptor and the expired-token check fire with a token that is already invalid, so the server refuses to bump sessionVersion on those paths — they were fine already
• the inactivity timer is the one that fires with a valid token — so one idle tab forgotten on another machine would have ended your active sessions on every device, and it would have looked like the UI randomly logging you out

Nobody chose that logout, so it should not reach beyond the browser it happened in. The endpoint now takes a scope: the inactivity path asks for a browser-local one (this browser's cookies cleared, nothing revoked server-side), while a logout you actually click stays account-wide — that part is doing exactly what it was built to do.

On the PR offer

Thank you — and I'll be honest about why I took this one in-house: beta-5.28.1 gets rebased and rewritten regularly (twice today alone), so your branch would have gone stale underneath you before it could merge. Your report did the hard half, and the commit carries this issue number.

Both fixes are in v5.28.1-beta.2, published today. Given the quality of your testing this week, I'd genuinely value a run of your scenarios against it — reopen this if anything survives.

[lidonius1122]

Sorry for the slow reply - I'm on holiday and only just got to a proper keyboard.

Ran both repros against v5.28.1-beta.2 and the fix holds - putting the old ordering back in a scratch copy brings the 500 straight back.

On the scoping - you're right that expired and stale tokens can't bump sessionVersion, I measured both. But the 30-day chain cap is one rejection that produces a 401 with a token the guard still accepts, since refreshToken checks it and validateUser doesn't. The interceptor treats that like any other 401 and calls the account-wide logout, so the server, seeing a valid token, does what it's asked: everything else signed in on that account gets kicked out at the same moment. Same shape as the inactivity timer, one endpoint further along.

aged token (sessionStartedAt -31d, exp +5m)
  GET /auth/check                200
  POST /auth/refresh             401   "Session has reached its maximum age"
  POST /auth/logout (no scope)   201   sessionVersion 0 -> 1
  second device                  200 -> 401

/auth/refresh isn't in SKIP_PATHS and refreshSession() doesn't clear the token first, so the interceptor fires with auth.token still set - checked in a TestBed, it calls logout() with no arguments. adminGuard refreshes on every admin-route navigation, so it lands on whichever admin page you happen to open that day. Rare, since the chain has to survive 30 days of regular use - though that part is a judgement call, I got there with a backdated sessionStartedAt rather than by waiting.

Happy to write up the reproduction if useful.

[bwp91]

Reproduced, and you are right on every step of that chain. Thank you for going back over it while on holiday - this is not something I would have found from a bug report.

Walking it against the code:

• validateUser() checks the user exists, the admin flag, and sessionVersion. It does not look at sessionStartedAt, so an aged token authenticates normally.
• refreshToken() is the only thing that reads it, and it rejects with a 401.
• SKIP_PATHS in the auth error interceptor was login, noauth, check. /auth/refresh was not in it.
• refreshSession() posts before clearing anything, so auth.token is still set when the 401 comes back.
• The interceptor therefore called logout() with no options, and the endpoint, handed a token that validates, did what it was asked and bumped sessionVersion.

Exactly the shape of the inactivity timer, one endpoint further along, as you say. And the admin guard refreshes on every admin-route navigation, so it is whichever admin page the user opens first.

What makes it worth fixing beyond this one rejection: every reason refreshToken() refuses is a reason to sign out this browser and nothing else.

Rejection	Other devices
User no longer exists	already rejected by validateUser
Permissions changed	already rejected - admin flag mismatch
Credentials changed	already rejected - sessionVersion mismatch
Wrong instance	never valid here anyway
Maximum age	legitimately still valid

So the first four gain nothing from an account-wide logout, and the last one is actively harmed by it. That made the fix a narrow one rather than a special case for the 30-day cap:

• /auth/refresh joins SKIP_PATHS, so the interceptor no longer decides the scope for an endpoint that can refuse a token the guard accepts
• refreshSession() owns that decision instead: on a 401 it asks for a browser-local logout and rethrows, and on anything else - a 500, a dropped connection - it does nothing at all, so a blip on the way to the server cannot sign anybody out

Two tests cover it, one per half, and each fails if its half is reverted.

Worth noting for the record that the comment in admin.guard.ts describes the old intent - that a demoted admin gets logged out through the rejection path. That still happens; it just stops reaching the user's other devices.

This is publishing as 5.28.1-beta.3 now, so it will be on npm shortly.

And yes please to the reproduction write-up, if you do not mind - a backdated sessionStartedAt is exactly the right way to get there, and having it as a test means the next person to touch this path cannot quietly undo it.

[lidonius1122]

I'd missed the dropped-connection half entirely.

Here it is, against v5.28.1-beta.3 - one test, in the the absolute session lifetime
block, next to the other signUserCookie ones:

    /** The bearer token from a cookie minted by `signUserCookie`. */
    function signUserToken(overrides: Record<string, any> = {}) {
      return signUserCookie(overrides).split('=').slice(1).join('=').split(';')[0]
    }

and the test itself:

    // #2981: `refreshToken()` is the only reader of `sessionStartedAt`, so a
    // token past the cap is refused a renewal while every guarded route still
    // accepts it. That is what let the browser turn a refused refresh into an
    // account-wide logout the server then honoured. The fix for that is in the
    // interceptor and `refreshSession()`, covered by the ui specs; this pins
    // the server side it reacts to, so the asymmetry cannot be dropped without
    // a test saying so.
    it('accepts an over-age token at /auth/check while refusing to renew it', async () => {
      const overAge = signUserToken({ sessionStartedAt: Math.floor(Date.now() / 1000) - (31 * DAY) })

      const checked = await app.inject({
        method: 'GET',
        path: '/auth/check',
        headers: { authorization: `Bearer ${overAge}` },
      })
      expect(checked.statusCode).toBe(200)

      const refreshed = await app.inject({
        method: 'POST',
        path: '/auth/refresh',
        headers: { authorization: `Bearer ${overAge}` },
      })
      expect(refreshed.statusCode).toBe(401)
      expect(refreshed.json().message).toContain('maximum age')
    })

62 passing in the auth e2e file, eslint clean. Mutations: adding the cap to
validateUser() fails this and nothing else in the suite; disabling it in refreshToken()
fails this plus refuses to renew a session past its maximum age and warns when it refuses a refresh, and stays quiet when it allows one. The maximum age assertion is there because a 401 from an expired or
malformed token would otherwise satisfy the same expectation.

One test I dropped, in case you wonder why it isn't here: an over-age token revoking the
account, scope: 'local' sparing the other device. It looked like it guarded the fix, but
/auth/logout never reads sessionStartedAt - it passed just as happily with a fresh
token, and duplicated keeps other sessions alive when the logout is scoped to this browser.

Your two ui specs cover the interceptor; they mock the 401 and can't reach the server side,
which is what this pins - so it fails out loud if that asymmetry ever changes.

The 31 days are a backdated sessionStartedAt rather than elapsed time, so it tests the
comparison and nothing about clocks.

Ran the original chain against a beta.3 instance too, driving the real interceptor rather
than a mock: {"scope":"local"} on the wire, second device still 200. Reverting both halves
in a scratch copy puts {} back and takes it to 401.

Happy to open a PR with it, or leave it with you - whichever is less work.

[bwp91]

Yes please, open the PR — and it should be yours rather than mine, because the reasoning in it is the part worth keeping.

Two things in particular. Dropping the logout test was the right call and for the right reason: /auth/logout never reads sessionStartedAt, so a test built on an over-age token there proves nothing the scoped-logout test does not already prove. A test that passes for the wrong reason is worse than no test. And running the original chain against a real beta.3 instance rather than a mock is the half I could not do from here — {"scope":"local"} on the wire with the second device still answering 200 is the actual proof the fix works, and it is worth having in the PR description.

Practical notes so it lands cleanly:

Target beta-5.28.1, not latest. I checked before saying so: MAX_SESSION_LIFETIME_SECONDS does not exist on latest at all, and neither does the the absolute session lifetime block — the whole cap is beta-only work so far. Against latest the test would have nothing to pin.

That does mean the same staleness risk I mentioned last time, but it is a very different proposition now: one test in one file, next to refuses to renew a session past its maximum age. If the branch moves under you, a rebase is a minute rather than an afternoon. I will merge it promptly rather than leave it sitting.

Nothing else needed — the changelog bullet is mine to add on merge.