Skip to content

Basic-mode teardown requests (KILL QUERY/export-cancel) get the wrong Authorization scheme after credentials clear #520

Description

@BorisTyshkevich

Problem

app.signOut's (and now, after #502, onAuthLost's) authenticated-session
teardown fires several best-effort authenticated requests fire-and-forget:
workbench.destroy()'s KILL QUERY, exportService.cancelExport()/
cancelExportScript()'s own KILL QUERY. These go through
net/ch-client.ts's authedFetch:

export async function authedFetch(ctx: ChCtx, url: string, sql: string, signal?: AbortSignal): Promise<Response> {
  const token = await ctx.getToken();
  if (!token) { ctx.onSignedOut(); throw new Error('not signed in'); }
  let bearer = token;
  let attempt = 0;
  const authHeader = ctx.authHeader || ((t: string) => 'Bearer ' + t);
  for (;;) {
    const resp = await ctx.fetch(url, { method: 'POST', body: sql, headers: { Authorization: authHeader(bearer) }, signal });
    ...

authHeader(bearer) is called only after await ctx.getToken() resolves —
i.e. in a microtask that resumes after the caller's own synchronous code has
finished. Both app.signOut and onAuthLost clear credentials
(clearTokens(), which unconditionally resets authMode to 'oauth') in the
same synchronous tick as firing this teardown, so by the time authHeader()
actually runs, authMode has already been reset.

For an HTTP-Basic-mode session (connectBasic), connection-session.ts's
authHeader:

function authHeader(t: string): string {
  if (authMode === 'basic') return 'Basic ' + t;
  if (chAuthVal !== 'basic') return 'Bearer ' + t;
  ...
}

sees the reset authMode === 'oauth' instead of 'basic', so it takes the
wrong branch and builds Authorization: Bearer <base64(user:pass)> — the
captured Basic credential sent under the wrong scheme — instead of
Authorization: Basic <base64(user:pass)>. ClickHouse almost certainly
rejects that, so the teardown's KILL QUERY/export-cancel requests silently
fail for Basic-mode users specifically, even though the token value itself
was captured correctly (this part of getToken()'s synchronous null-check
runs before the reset, and is unaffected).

Impact

A Basic-auth user whose session ends (explicit sign-out or involuntary auth
loss) may leave an in-flight query, export, or stream running server-side
past sign-out/login — the exact failure mode #502 addressed, but only for
this one path, and only for Basic-mode sessions.

Scope note

Pre-existing in app.signOut's teardown before #502; #502 made onAuthLost
consistent with it, not worse. Not fixed as part of #502 because it needs a
real design decision (e.g. capturing the Authorization header eagerly,
before the fire-and-forget call, rather than resolving it lazily inside
authedFetch), not a one-line reorder.

Suggested fix

Capture ctx.authHeader(token) synchronously (before firing the
fire-and-forget teardown call, or before clearing credentials) rather than
resolving it lazily inside authedFetch's retry loop — or accept a
pre-built Authorization header as an argument for teardown-only callers.

Metadata

Metadata

Assignees

No one assigned

    Labels

    inboxFiled mid-task; not yet triaged into the roadmap

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions