Concurrent token refreshes can invalidate rotating refresh-token sessions

[BorisTyshkevich]

Problem

ConnectionSession has no single-flight protection around OAuth refresh. Both getToken() and chCtx.refresh can start refresh() independently, and refresh() redeems the current refreshTok directly:

async function refresh(): Promise<boolean> {
  if (authMode === 'basic') return false;
  const cfg = await resolveConfig();
  const tokens = await refreshTokens(fetchFn, cfg, refreshTok);
  const bearer = bearerFromTokens(tokens, cfg.bearer);
  if (!bearer) return false;
  setTokens(bearer, tokens?.refresh_token);
  return true;
}

getToken() then clears the whole session when its refresh attempt returns false:

if (await refresh()) return token;
clearTokens();
return null;

Two concurrent callers that observe the same expired token therefore redeem the same refresh token in parallel.

The code already documents this hazard in ensureFreshToken():

a rotating refresh token used N-ways at once would invalidate itself

However, ensureFreshToken() is only a per-call-site preflight. It does not make refresh itself single-flight, and it is not used by bootstrap despite the adjacent comment saying that it is.

Reproduction path

A deterministic unit reproduction can use an expired access token and a rotating, single-use refresh token:

1. Call session.getToken() twice before either token-endpoint request resolves.
2. Assert that both requests carry the same old refresh token.
3. Resolve one request successfully with a new access token and rotated refresh token.
4. Reject the other request as invalid_grant.
5. The successful caller commits the new session through setTokens().
6. The losing caller returns false, runs clearTokens(), and can subsequently trigger onSignedOut() through authedFetch.

Depending on completion order, the loser either wipes the winner's newly committed tokens or forces the login screen while the winner later restores token state behind it.

Bootstrap exposure

Bootstrap checks isSignedIn() with zero expiry skew, then calls only ensureConfig() before starting the initial application work:

await app.conn.ensureConfig();
await app.loadWorkspaceOnBoot();
app.renderCurrentSurface();
void app.catalog.loadVersion();

Mounting the application shell starts both catalog.loadSchema() and catalog.loadReference() without awaiting either. loadVersion() is then started separately. Each path eventually reaches authedFetch() and getToken().

A token that is technically valid for isSignedIn() but within getToken()'s refresh window can therefore produce three concurrent refresh attempts during normal startup.

Other unprotected fan-outs also exist. For example, loadTableDetail() starts four ClickHouse reads with Promise.all() and its SchemaGraphSession.loadNodeDetail() caller performs no token preflight.

Some reported internal fan-outs are less direct than this: loadSchema() performs an initial sequential query before its later Promise.all(), and schema-lineage entry points currently preflight with getToken(). They should not be relied on as the primary reproduction, but central single-flight protection is still preferable to auditing every current and future fan-out.

Impact

This affects IdPs that rotate or otherwise make refresh tokens single-use. Non-rotating providers may tolerate the duplicate requests.

For affected deployments, a healthy session can be cleared and the user returned to login during startup or another concurrent request wave. The race can also leave UI/auth state inconsistent if a successful refresh settles after a losing caller has already rendered login.

Suggested fix

Make refresh single-flight inside ConnectionSession, at the refresh() level rather than only in selected callers. This covers both getToken() and authedFetch()'s direct ctx.refresh() retry path.

Conceptually:

let refreshInFlight: Promise<boolean> | null = null;

function refresh(): Promise<boolean> {
  if (authMode === 'basic') return Promise.resolve(false);
  if (refreshInFlight) return refreshInFlight;

  const operation = (async () => {
    const cfg = await resolveConfig();
    const tokens = await refreshTokens(fetchFn, cfg, refreshTok);
    const bearer = bearerFromTokens(tokens, cfg.bearer);
    if (!bearer) return false;
    setTokens(bearer, tokens?.refresh_token);
    return true;
  })();

  refreshInFlight = operation.finally(() => {
    if (refreshInFlight === operation) refreshInFlight = null;
  });
  return refreshInFlight;
}

The exact implementation should use identity-safe cleanup and should ensure explicit sign-out invalidates an in-flight refresh so a late response cannot resurrect the session.

ensureFreshToken() can remain as a useful fan-out preflight, but correctness should not depend on every call site remembering to use it. Update its stale bootstrap comment as part of the fix.

Tests

• Launch two concurrent getToken() calls with an expired token and assert that the token endpoint is called once.
• Return a rotated refresh token and assert both callers receive the same fresh access token and the rotated token remains stored.
• Exercise concurrent getToken() and direct chCtx.refresh() calls and assert one refresh request.
• Verify a failed shared refresh clears tokens once and produces a consistent result for every waiter.
• Sign out while refresh is pending, resolve the token endpoint successfully, and assert the session is not resurrected.
• Add a bootstrap regression test with deferred token refresh and simultaneous schema/reference/version loads.

[BorisTyshkevich]

Resolved by #523.

ConnectionSession.refresh() is now single-flight per credential epoch. Concurrent getToken() callers and direct chCtx.refresh() callers share the same refresh promise, so a rotating refresh token is redeemed only once.

Refresh settlement is also epoch-fenced: a newer login or explicit sign-out advances the credential epoch, preventing a late refresh response from restoring or overwriting credentials. Identity-safe slot cleanup prevents an older refresh from clearing a newer epoch’s slot.

The concurrent-refresh, rotated-token, stale-settlement, and late-cleanup paths are covered in tests/unit/connection-session.test.ts.

The original bootstrap fan-out is therefore no longer a correctness issue because refresh serialization is enforced centrally rather than at individual call sites.