Do not let a session outlive the account it belongs to - #834
Merged
Merged
Conversation
Disabling an account did not end the session it was already using. The API re-reads the user on every request and refuses a disabled one (Api::setUserData()), and the web login refuses one too, but nothing revisited it on any later web request: the session held whatever had been true at login. Nor does such a session lapse on its own. The timeout is measured from the last request, so the session being actively used is precisely the one that never expires — and an account is normally disabled because of what is being done with it right now. The administrator disables the account, watches the API token stop working, and the browser session carries on. Init now re-reads the account on each request of a signed-in session and ends it with SessionLifecycleHandler::restart(), the same thing the timeout already does, so the user lands on the login page as after any other expiry. It sits beside the timeout check because it is the same question with the same answer, and costs one primary-key read per authenticated request, which is what the API has always paid. Only a positive answer ends a session. A read that fails — the database briefly unreachable, a row that does not come back — says nothing about the account, and this runs on every request of every session: answering "disabled" to a hiccup would sign out every user at once and turn a blip into an outage. An account deleted rather than disabled is left to the ordinary expiry for the same reason. The check reads isDisabled() === true rather than the value: the getter is ?bool, and under strict_types a row whose flag was never set is a TypeError out of a method declared bool, on every request of every session. Three tests in InitSessionTest drive the full initialize() with a genuinely returning session — signed in, written out, then picked up the way a second request does: a disabled account's session ends, an enabled account's survives, and a failed read leaves the session alone. Removing the guard fails only the first. CLAUDE.md records a gap found while looking at this and deliberately not fixed: POST /api/v1/auth-tokens answers 500 for every action whose token carries a vault, because the API can only load the master password from the calling token's own vault and AUTHTOKEN_CREATE has none. Making it work means deciding that a token which can mint tokens also carries the master password, which is a product decision rather than a bug fix.
This was referenced Aug 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
Disabling an account did not end the session it was already using.
isDisabledApi::setUserData())LoginUser)The web trusted what login had put in the session, and nothing ever revisited it. Nor does the
session lapse on its own: the timeout is measured from the last request, so the session that is
being actively used is precisely the one that never expires — and an account is normally disabled
because of what is being done with it right now.
So the administrator disables the account, watches the API token stop working, and the browser
session carries on.
The fix
Initre-reads the account on each request of a signed-in session and ends the session if it hasbeen disabled, using
SessionLifecycleHandler::restart()— the same thing the timeout already does,so the user lands on the login page exactly as after any other expiry. It sits beside the timeout
check, because it is the same question (may this session continue) with the same answer, and costs
one primary-key read per authenticated request, which is what the API has always paid.
Only a positive answer ends a session. A read that fails — the database briefly unreachable, a
row that does not come back — says nothing about the account, and this runs on every request of
every session: answering "disabled" to a hiccup would sign out every user at once and turn a blip
into an outage. An account that has been deleted rather than disabled is left to the ordinary
expiry for the same reason. That trade is deliberate and has its own test.
Test
Three tests in
InitSessionTest, driven through the fullInit::initialize()with a genuinelyreturning session — signed in, written out, then picked up the way a real second request does,
rather than handed a session that was never stored:
session, which is not a check on the account at all;
Mutation-checked: removing the guard fails exactly the first and leaves both controls passing.
PHPStan level 6 and PHPCS clean.
One thing worth flagging
The first version of this guard declared
: booland returnedUser::isDisabled(), which is?bool. Understrict_typesa row whose flag is null is then aTypeError— out of a method thatruns on every request of every session. The integration suite caught it as
"Init::isUserDisabled(): Return value must be of type bool, null returned"on unrelated tests. Itis
=== truenow, with a comment, because "the flag was never set" is not "the account is disabled".The integration suite also caught the guard ending sessions in tests where nothing was disabled: the
harness answers one
Userrow for both the signed-in user and the user under test, and an emptyresult read as "account gone". That is what settled the failure mode above — and the suite now runs
with exactly the baseline's warning count rather than three extra.
Also
CLAUDE.mdgains a Known gap section recording something found while looking at this anddeliberately not fixed:
POST /api/v1/auth-tokensanswers 500 for every action whose token carriesa vault —
ACCOUNT_VIEWandACCOUNT_CREATEamong them — because the API can only load the masterpassword from the calling token's own vault, and
AUTHTOKEN_CREATEhas none. Making it work meansdeciding that a token which can mint tokens also carries the master password, which is a product
decision rather than a bug fix, so it is written down with the diagnosis instead of patched.