Skip to content

TCHAP: invalidate client on account expiry#1599

Open
thistehneisen wants to merge 1 commit into
tchapgouv:develop_tchapfrom
thistehneisen:fix-expired-account-session-teardown
Open

TCHAP: invalidate client on account expiry#1599
thistehneisen wants to merge 1 commit into
tchapgouv:develop_tchapfrom
thistehneisen:fix-expired-account-session-teardown

Conversation

@thistehneisen
Copy link
Copy Markdown

Fix: tear down the Matrix client on account expiry instead of keeping it live

Summary

When the homeserver reports an expired account
(HttpApiEvent.ORG_MATRIX_EXPIRED_ACCOUNT, from the
synapse-email-account-validity module), ExpiredAccountHandler called:

//shutdown all matrix react services, but without unsetting the client
stopMatrixClient(false);

With unsetClient = false, Lifecycle.stopMatrixClient() only stops the sync
loop and React services. It does not call MatrixClientPeg.unset() or
cli.store.destroy(), so after expiry:

  • the MatrixClient and its in-memory, already-decrypted room/crypto store
    stay fully resident and reachable (e.g. mxMatrixClientPeg.get().getRooms()),
  • the OAuth access token is still valid and still attached to a usable client,

and the only thing enforcing expiry is the non-cancelable ExpiredAccountDialog
React overlay.

Impact

An expired account is meant to be revoked (account-lifecycle / off-boarding).
Because the session is only "paused" and never invalidated client-side, an
expired user can, in the still-open tab:

  • read the entire already-synced decrypted message store with no further
    server calls, and
  • reuse the live authenticated client / token against any endpoint the
    server-side account-validity module does not gate.

The control is effectively cosmetic. Confidentiality of already-synced
content is the primary impact.

Why it wasn't simply stopMatrixClient(true)

The renewal flow legitimately needs an authenticated call after expiry:

  • TchapUtils.requestNewExpiredAccountEmail() — POST account_validity/send_mail
  • TchapUtils.isAccountExpired() — re-check used by onBeforeClose

Both used MatrixClientPeg.safeGet(), which throws once the client is unset.
That coupling is why the client was kept alive — and why the hole existed.

Fix

Decouple the renewal flow from the live client, then tear the client down
properly:

  1. ExpiredAccountHandler.onExpiredAccountError() now captures the minimal
    credentials (homeserverUrl, accessToken, userId) before teardown via
    TchapUtils.setExpiredAccountCredentials(...), then calls
    stopMatrixClient(true) — which unsets the peg and destroys the in-memory
    (decrypted) room/crypto store.
  2. requestNewExpiredAccountEmail() and isAccountExpired() no longer touch the
    Matrix client. They use the captured credentials and raw fetch:
    • isAccountExpired() now does an authenticated GET on the profile endpoint
      and checks the ORG_MATRIX_EXPIRED_ACCOUNT errcode (same signal the old
      getProfileInfo() relied on), so it works with no client/store.
    • A fallback to a still-live client is kept for any call outside the expiry
      flow (behaviour unchanged there).
  3. On successful renewal the existing PlatformPeg.get().reload() rebuilds a
    fresh client from the stored session, so the user experience is unchanged.

Residual risk (intentional / out of scope)

This does not clear the persisted session (localStorage/IndexedDB) or revoke
the token server-side — doing so would force a full re-login after every renewal
and is the server's responsibility (the account-validity module). The token is
still held transiently in JS memory while the expiry dialog is open, scoped to
the two account_validity endpoints, and discarded on reload. The concrete
improvements are: the resident decrypted message/crypto store is destroyed,
the live client and mxMatrixClientPeg access are removed, and credential
exposure is reduced to an ephemeral, endpoint-scoped token.

Changes

  • src/tchap/lib/ExpiredAccountHandler.ts — capture creds before teardown;
    stopMatrixClient(false)stopMatrixClient(true).
  • src/tchap/util/TchapUtils.ts — credential holder + setter/getter; rewrite
    requestNewExpiredAccountEmail() / isAccountExpired() to be client-free;
    drop now-unused MatrixError import.
  • src/tchap/util/TchapApi.ts — add profileUrl constant.

Testing

  • Trigger account expiry: dialog appears; mxMatrixClientPeg.get() is now
    null and getRooms()/decrypted timelines are no longer reachable.
  • "Request new email" from the expired dialog still sends the renewal mail.
  • After renewing the account, the dialog closes and the app reloads into a
    working, logged-in session.
  • Non-expired account: normal operation unaffected (fallback path).

Nils Putnins / OffSeq Cybersecurity
npu@offseq.com / https://offseq.com / https://radar.offseq.com

On ORG_MATRIX_EXPIRED_ACCOUNT the handler called stopMatrixClient(false),
which keeps the MatrixClient, its in-memory decrypted room/crypto store and
the access token fully live; enforcement was only the expired-account modal.
An expired account could therefore still read already-synced decrypted
content and reuse the authenticated client.

Capture the minimal credentials needed by the account_validity endpoints
before teardown, then call stopMatrixClient(true) to unset the client and
destroy the in-memory store. requestNewExpiredAccountEmail() and
isAccountExpired() no longer depend on a live client (raw fetch using the
captured credentials), so the renewal flow keeps working.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant