Skip to content

A failed IndexedDB token write can silently and permanently lose the session (and leave it orphaned on the server) #34866

Description

@hughns

When persistTokenInStorage() cannot write a token to IndexedDB, it falls back to writing it to localStorage. However, precedence is given to IndexedDB and the fallback is only used if IndexedDB does not contain a value for the token. Therefore, if IndexedDB contains an older (stale) token it will still be used.

With rotating refresh tokens (any MAS/OAuth 2.0 deployment) that is fatal: the stale refresh token has already been consumed, the server rejects it with a 400, and the client responds by deleting all local data and forcing a re-login with a new device ID.

Neither the write failure nor the fallback is logged, so the moment a session becomes doomed leaves no trace in rageshakes at all. The failure surfaces on a later launch as an unexplained logout.

Mechanism

The write path:

try {
// Save either the encrypted access token, or the plain access
// token if there is no token or we were unable to encrypt (e.g. if the browser doesn't
// have WebCrypto).
await StorageAccess.idbSave("account", storageKey, encryptedToken || token);
} catch {
// if we couldn't save to indexedDB, fall back to localStorage. We
// store the access token unencrypted since localStorage only saves
// strings.
if (!!token) {
localStorage.setItem(storageKey, token);
} else {
localStorage.removeItem(storageKey);
}
}
} else {
try {
await StorageAccess.idbSave("account", storageKey, token);
} catch {
if (!!token) {
localStorage.setItem(storageKey, token);
} else {
localStorage.removeItem(storageKey);
}
}
}

The read path:

let token: string | undefined;
try {
token = await StorageAccess.idbLoad("account", storageKey);
} catch (e) {
logger.error(`StorageManager.idbLoad failed for account:${storageKey}`, e);
}
if (!token) {
token = localStorage.getItem(storageKey) ?? undefined;
if (token) {
try {
// try to migrate access token to IndexedDB if we can
await StorageAccess.idbSave("account", storageKey, token);
localStorage.removeItem(storageKey);
} catch (e) {
logger.error(`migration of token ${storageKey} to IndexedDB failed`, e);
}
}
}

Note that there is no logging on either path.

The problem

Failure sequence:

  • Refresh succeeds; the server rotates RT1 → RT2 and invalidates RT1.
  • persistTokens()idbSave() fails. RT2 goes to localStorage; IndexedDB still holds RT1.
  • The current runtime keeps working — it has RT2 in memory — so nothing appears wrong.

If the app stays alive until the next token refresh and that succeeds in being written to IndexedDB then the failure has self rectified.

However, if the app is closed:

  • On the next launch, getStoredToken() returns RT1 from IndexedDB. The localStorage copy is never consulted.
  • POST /oauth2/token gets a 400 Bad Request (because RT1 already consumed).
  • matrix-js-sdk raises TokenRefreshLogoutErrorHttpApiEvent.SessionLoggedOut.
  • This is treated as a hard failure and calls: clearStorage({ deleteEverything: true }).

The user loses their crypto store, has to re-verify, and gets a new device ID. The old device is left behind on the server, because by then the only tokens the client can offer to /oauth2/revoke are the dead ones (RFC 7009 returns 200 for an unknown token, so the revocation is a silent no-op).

Secondary problem: a plaintext token is left behind indefinitely

The fallback writes the token to localStorage unencrypted (localStorage only stores strings), and the write path never clears it after a later successful IndexedDB write. Since the read path can no longer reach it, that value simply stays there — an unencrypted access or refresh token, in a profile where the pickle key exists specifically so that tokens are encrypted at rest. Nothing removes it short of clearStorage() on logout.

Proposed fix

  • Write the fallback to a distinct key (e.g. <key>_fallback) and prefer it on read. It is only ever written when an IndexedDB write failed, and is cleared as soon as one succeeds, so it is always at least as new as IndexedDB. Keeping it separate from the primary key preserves the existing meaning of a value there — a pre-IndexedDB legacy token, which may be older than IndexedDB — so the migration path is unaffected and the change is safe on upgrade.
  • Log the write failure. This is the single highest-value part: it makes this class of report self-diagnosing instead of invisible.
  • Clear the stale IndexedDB entry after a failed write, so a client that does not know about the fallback key reads "no token" rather than an outdated one.
  • Sweep up any plaintext token still sitting at the primary key once IndexedDB has answered, and log it. Deliberately without using it: the old code did not clear it on a later successful write, so it may be older than the IndexedDB copy and preferring it could demote a working session to a dead token.

Not covered by the above fix: Users who are already in the broken state — stale token in IndexedDB, good one in localStorage — are not recovered by this. Preferring the old value is not safe, because coexistence does not tell you which of the two is newer.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions