From 1b163018ea593f382ef284eeddd3c3c43a285051 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 20:54:49 +1000 Subject: [PATCH 01/15] fix(api): detect HTTP status from axios errors in getApplications, getEnvironments and getEnvironmentMetrics The quant-client SDK uses axios, which sets error.response.status, not error.statusCode. These three methods checked only statusCode, so a 401 showed 'Failed to fetch ...' instead of the 'run qc login' message. --- __tests__/utils/api.test.ts | 21 +++++++++++++++++++ src/utils/api.ts | 40 ++++++++++++++++++++++++------------- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/__tests__/utils/api.test.ts b/__tests__/utils/api.test.ts index 7f04b13..6de69eb 100644 --- a/__tests__/utils/api.test.ts +++ b/__tests__/utils/api.test.ts @@ -536,4 +536,25 @@ describe('ApiClient', () => { await expect(client.getOrganizations()).rejects.toThrow(/not yet implemented/); }); }); + describe('axios errors (error.response.status)', () => { + const axiosError = (status: number) => ({ message: `Request failed with status code ${status}`, response: { status } }); + + it('getApplications maps an axios 401 to the login message', async () => { + mockListApplications.mockRejectedValue(axiosError(401)); + const client = new ApiClient('https://api.quantcdn.io', 'token', 'org-1'); + await expect(client.getApplications()).rejects.toThrow(/Authentication expired/); + }); + + it('getEnvironments maps an axios 403 to access denied', async () => { + mockListEnvironments.mockRejectedValue(axiosError(403)); + const client = new ApiClient('https://api.quantcdn.io', 'token', 'org-1', 'app-1'); + await expect(client.getEnvironments()).rejects.toThrow(/Access denied/); + }); + + it('getEnvironmentMetrics maps an axios 401 to the login message', async () => { + mockGetEnvironmentMetrics.mockRejectedValue(axiosError(401)); + const client = new ApiClient('https://api.quantcdn.io', 'token', 'org-1', 'app-1'); + await expect(client.getEnvironmentMetrics({ environmentId: 'prod' })).rejects.toThrow(/Authentication expired/); + }); + }); }); diff --git a/src/utils/api.ts b/src/utils/api.ts index 2c3e561..cb74587 100644 --- a/src/utils/api.ts +++ b/src/utils/api.ts @@ -10,6 +10,19 @@ export interface ApiOptions { environmentId?: string; } +/** The HTTP status of an SDK error: axios sets response.status, older clients set statusCode. */ +function httpStatus(error: any): number | undefined { + return error?.statusCode ?? error?.response?.status; +} + +function environmentsNotFoundMessage(error: { body?: { message?: string } }, organizationId: string, applicationId: string): string { + const message: string = error.body?.message ?? ''; + if (message.includes('Application') && message.includes('not found')) { + return `Application '${organizationId}/${applicationId}' not found.`; + } + return `Organization '${organizationId}' not found or no access to environments.`; +} + export class ApiClient { private applicationsApi: ApplicationsApi; public environmentsApi: EnvironmentsApi; @@ -80,12 +93,13 @@ export class ApiClient { const response = await this.applicationsApi.listApplications(organizationId); return response.data; } catch (error: any) { + const status = httpStatus(error); // Provide friendly error messages instead of debug dumps - if (error.statusCode === 404) { + if (status === 404) { throw new Error(`Organization '${organizationId}' not found or no access to applications.`); - } else if (error.statusCode === 403) { + } else if (status === 403) { throw new Error(`Access denied to applications in organization '${organizationId}'.`); - } else if (error.statusCode === 401) { + } else if (status === 401) { throw new Error('Authentication expired. Please run `qc login` to re-authenticate.'); } else { throw new Error(`Failed to fetch applications: ${error.message || 'Network error'}`); @@ -109,16 +123,13 @@ export class ApiClient { const response = await this.environmentsApi.listEnvironments(organizationId, applicationId); return response.data; } catch (error: any) { + const status = httpStatus(error); // Provide friendly error messages instead of debug dumps - if (error.statusCode === 404) { - if (error.body?.message?.includes('Application') && error.body?.message?.includes('not found')) { - throw new Error(`Application '${organizationId}/${applicationId}' not found.`); - } else { - throw new Error(`Organization '${organizationId}' not found or no access to environments.`); - } - } else if (error.statusCode === 403) { + if (status === 404) { + throw new Error(environmentsNotFoundMessage(error, organizationId, applicationId)); + } else if (status === 403) { throw new Error(`Access denied to environments in '${organizationId}/${applicationId}'.`); - } else if (error.statusCode === 401) { + } else if (status === 401) { throw new Error('Authentication expired. Please run `qc login` to re-authenticate.'); } else { throw new Error(`Failed to fetch environments: ${error.message || 'Network error'}`); @@ -179,12 +190,13 @@ export class ApiClient { ); return response.data; } catch (error: any) { + const status = httpStatus(error); // Provide friendly error messages instead of debug dumps - if (error.statusCode === 404) { + if (status === 404) { throw new Error(`Environment '${organizationId}/${applicationId}/${options.environmentId}' not found or metrics unavailable.`); - } else if (error.statusCode === 403) { + } else if (status === 403) { throw new Error(`Access denied to metrics for '${organizationId}/${applicationId}/${options.environmentId}'.`); - } else if (error.statusCode === 401) { + } else if (status === 401) { throw new Error('Authentication expired. Please run `qc login` to re-authenticate.'); } else { throw new Error(`Failed to fetch metrics: ${error.message || 'Network error'}`); From 56c24db765086fd16becd2a28c1b2c3b3241ef68 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 20:55:01 +1000 Subject: [PATCH 02/15] feat(auth): refresh OAuth access tokens with the refresh token The portal will issue quant-cli logins with a 90-day access token and a rotating refresh token. qc now: - refreshes the access token when 7 days or less are left, before the command runs (grant_type=refresh_token, client_id=quant-cli); - after a 401 from the v3 API (axios interceptor) or /api/oauth/user, refreshes once and repeats the request once, with no loop; - holds a lock file next to the credentials file (fs.open 'wx', 30 s stale timeout) while it refreshes, and reloads the stored login first. If another qc process already rotated the token, qc uses that result and sends nothing, so one refresh token is never sent twice (the portal revokes the whole chain on reuse); - keeps the stored refresh token when a response has none; - on invalid_grant/invalid_client, tells the user to run `qc login` and keeps the credentials file; - writes the credentials file atomically (temp file, mode 0600, rename), and takes the same lock for every read-modify-write of that file. Legacy logins have no refresh token or expiry. They are never refreshed, and their API clients keep the default axios with no retry. --- __tests__/utils/context.test.ts | 2 + __tests__/utils/oauth-refresh-flow.test.ts | 231 +++++++++++++++++++++ __tests__/utils/token-refresh.test.ts | 134 ++++++++++++ src/utils/api.ts | 77 +++++-- src/utils/auth-retry.ts | 34 +++ src/utils/config.ts | 51 ++++- src/utils/context.ts | 76 ++++++- src/utils/file-lock.ts | 66 ++++++ src/utils/token-refresh.ts | 182 ++++++++++++++++ 9 files changed, 817 insertions(+), 36 deletions(-) create mode 100644 __tests__/utils/oauth-refresh-flow.test.ts create mode 100644 __tests__/utils/token-refresh.test.ts create mode 100644 src/utils/auth-retry.ts create mode 100644 src/utils/file-lock.ts create mode 100644 src/utils/token-refresh.ts diff --git a/__tests__/utils/context.test.ts b/__tests__/utils/context.test.ts index 2e04187..68cbf9f 100644 --- a/__tests__/utils/context.test.ts +++ b/__tests__/utils/context.test.ts @@ -9,6 +9,8 @@ jest.unstable_mockModule('../../src/utils/config.js', () => ({ loadMultiPlatformConfig: mockLoadMulti, saveActivePlatformConfig: mockSaveActive, savePlatformUpdates: mockSaveNamed, + applyPlatformUpdates: mockSaveNamed, + CREDENTIALS_LOCK_FILE: '/nonexistent/credentials.lock', getActivePlatformConfig: async () => { const m = await mockLoadMulti(); const { platformInfo, ...auth } = m.platforms[m.activePlatform]; diff --git a/__tests__/utils/oauth-refresh-flow.test.ts b/__tests__/utils/oauth-refresh-flow.test.ts new file mode 100644 index 0000000..b22fad8 --- /dev/null +++ b/__tests__/utils/oauth-refresh-flow.test.ts @@ -0,0 +1,231 @@ +import { describe, it, expect, jest, beforeAll, beforeEach, afterEach, afterAll } from '@jest/globals'; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { createRequire } from 'module'; +import axios, { AxiosError, AxiosResponse, InternalAxiosRequestConfig } from 'axios'; + +// Real credentials file, lock file and refresh code, in a temp config dir. +const home = mkdtempSync(join(tmpdir(), 'qc-oauth-refresh-')); +process.env.QUANT_CONFIG_DIR = join(home, '.quant'); +mkdirSync(process.env.QUANT_CONFIG_DIR); +if (!process.env.QUANT_CONFIG_DIR.startsWith(home)) { + throw new Error('refusing to run: QUANT_CONFIG_DIR is not the temp dir'); +} + +jest.unstable_mockModule('../../src/utils/project-config.js', () => ({ + getProjectConfig: () => ({}), +})); + +const HOST = 'https://dash.example.test'; +const DAY = 24 * 60 * 60 * 1000; + +let credFile: string; +let ctx: typeof import('../../src/utils/context.js'); +let ApiClient: typeof import('../../src/utils/api.js').ApiClient; + +beforeAll(async () => { + ({ CONFIG_FILE: credFile } = await import('../../src/utils/config.js')); + ctx = await import('../../src/utils/context.js'); + ({ ApiClient } = await import('../../src/utils/api.js')); +}); + +afterAll(() => rmSync(home, { recursive: true, force: true })); + +interface Login { + token: string; + refreshToken?: string; + expiresAt?: string; +} + +function writeLogin(login: Login): void { + const platform = { ...login, host: HOST, activeOrganization: 'acme', platformInfo: { id: 'quantcdn', name: 'Q', host: HOST } }; + writeFileSync(credFile, JSON.stringify({ activePlatform: 'quantcdn', platforms: { quantcdn: platform } })); +} + +const stored = () => JSON.parse(readFileSync(credFile, 'utf-8')).platforms.quantcdn; + +const json = (status: number, body: unknown) => ({ + ok: status >= 200 && status < 300, + status, + statusText: String(status), + json: async () => body, +}); + +const rotated = (n: number, extra: Record = {}) => + json(200, { access_token: `qc_new${n}`, refresh_token: `qcr_new${n}`, expires_in: 90 * 86400, token_type: 'Bearer', ...extra }); + +const fetchMock = jest.fn<(...args: any[]) => any>(); +const isTokenCall = (call: any[]) => String(call[0]).endsWith('/oauth/token'); +const tokenCalls = () => fetchMock.mock.calls.filter(isTokenCall); +const sentRefreshToken = (call: any[]) => new URLSearchParams(call[1].body).get('refresh_token'); + +// The SDK calls the CommonJS axios when no instance is passed (legacy logins). +const cjsAxios = createRequire(import.meta.url)('axios'); +const sdkAxios = cjsAxios.default ?? cjsAxios; + +const realFetch = global.fetch; +const realAdapter = axios.defaults.adapter; +const realSdkAdapter = sdkAxios.defaults.adapter; + +beforeEach(() => { + jest.clearAllMocks(); + fetchMock.mockReset(); + global.fetch = fetchMock as any; +}); + +afterEach(() => { + global.fetch = realFetch; + axios.defaults.adapter = realAdapter; + sdkAxios.defaults.adapter = realSdkAdapter; + jest.restoreAllMocks(); +}); + +/** Routes the quant-client (axios) requests to respond(), and records the token sent. */ +function fakeApi(respond: (token: string) => number): string[] { + const tokens: string[] = []; + const adapter = async (config: InternalAxiosRequestConfig) => { + const token = String(config.headers.Authorization).replace('Bearer ', ''); + tokens.push(token); + const status = respond(token); + const response: AxiosResponse = { data: [{ name: 'app' }], status, statusText: String(status), headers: {}, config }; + if (status >= 400) { + throw new AxiosError(`Request failed with status code ${status}`, 'ERR_BAD_REQUEST', config, null, response); + } + return response; + }; + axios.defaults.adapter = adapter; + sdkAxios.defaults.adapter = adapter; + return tokens; +} + +describe('legacy login (no refresh token, no expiry)', () => { + it('is never refreshed, and a 401 is not retried', async () => { + writeLogin({ token: 'legacy-token' }); + const before = readFileSync(credFile, 'utf-8'); + const sent = fakeApi(() => 401); + + const auth = await ctx.getEffectiveAuth(); + const client = await ApiClient.create(); + await expect(client.getApplications()).rejects.toThrow(/Authentication expired/); + + expect(auth?.token).toBe('legacy-token'); + expect(sent).toEqual(['legacy-token']); + expect(fetchMock).not.toHaveBeenCalled(); + expect(readFileSync(credFile, 'utf-8')).toBe(before); + }); +}); + +describe('proactive refresh', () => { + it('refreshes a due token and saves the new pair atomically with mode 0600', async () => { + writeLogin({ token: 'qc_old1', refreshToken: 'qcr_old1', expiresAt: new Date(Date.now() + DAY).toISOString() }); + fetchMock.mockResolvedValue(rotated(1)); + + const auth = await ctx.getEffectiveAuth(); + + expect(auth?.token).toBe('qc_new1'); + expect(tokenCalls()).toHaveLength(1); + expect(sentRefreshToken(tokenCalls()[0])).toBe('qcr_old1'); + expect(stored()).toMatchObject({ token: 'qc_new1', refreshToken: 'qcr_new1', activeOrganization: 'acme' }); + expect(Date.parse(stored().expiresAt)).toBeGreaterThan(Date.now() + 89 * DAY); + expect(statSync(credFile).mode & 0o777).toBe(0o600); + expect(readdirSync(join(home, '.quant'))).toEqual(['credentials']); + }); + + it('does not refresh a token with more than 7 days left', async () => { + writeLogin({ token: 'qc_fresh', refreshToken: 'qcr_fresh', expiresAt: new Date(Date.now() + 30 * DAY).toISOString() }); + + expect((await ctx.getEffectiveAuth())?.token).toBe('qc_fresh'); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('keeps the stored refresh token when the response has none', async () => { + writeLogin({ token: 'qc_old2', refreshToken: 'qcr_keep', expiresAt: new Date(Date.now() + DAY).toISOString() }); + fetchMock.mockResolvedValue(json(200, { access_token: 'qc_new2', expires_in: 90 * 86400, token_type: 'Bearer' })); + + await ctx.getEffectiveAuth(); + + expect(stored()).toMatchObject({ token: 'qc_new2', refreshToken: 'qcr_keep' }); + }); + + it('sends one refresh request when two refreshes run at the same time', async () => { + writeLogin({ token: 'qc_old3', refreshToken: 'qcr_old3', expiresAt: new Date(Date.now() - DAY).toISOString() }); + fetchMock.mockImplementation(async () => { + await new Promise((resolve) => setTimeout(resolve, 150)); + return rotated(3); + }); + + const [a, b] = await Promise.all([ctx.getEffectiveAuth(), ctx.getEffectiveAuth()]); + + expect(tokenCalls()).toHaveLength(1); + expect(a?.token).toBe('qc_new3'); + expect(b?.token).toBe('qc_new3'); + expect(stored().refreshToken).toBe('qcr_new3'); + expect(existsSync(`${credFile}.lock`)).toBe(false); + }); +}); + +describe('401 retry', () => { + const notDue = () => new Date(Date.now() + 60 * DAY).toISOString(); + + it('refreshes once and repeats the API request once', async () => { + writeLogin({ token: 'qc_old4', refreshToken: 'qcr_old4', expiresAt: notDue() }); + fetchMock.mockResolvedValue(rotated(4)); + const sent = fakeApi((token) => (token === 'qc_new4' ? 200 : 401)); + + const client = await ApiClient.create(); + const apps = await client.getApplications(); + + expect(apps).toEqual([{ name: 'app' }]); + expect(sent).toEqual(['qc_old4', 'qc_new4']); + expect(tokenCalls()).toHaveLength(1); + expect(stored().token).toBe('qc_new4'); + }); + + it('does not loop when the renewed token also gets 401', async () => { + writeLogin({ token: 'qc_old5', refreshToken: 'qcr_old5', expiresAt: notDue() }); + fetchMock.mockResolvedValue(rotated(5)); + const sent = fakeApi(() => 401); + + const client = await ApiClient.create(); + await expect(client.getApplications()).rejects.toThrow(/Authentication expired/); + + expect(sent).toEqual(['qc_old5', 'qc_new5']); + expect(tokenCalls()).toHaveLength(1); + }); + + it('refreshes once and repeats the /api/oauth/user request once', async () => { + writeLogin({ token: 'qc_old6', refreshToken: 'qcr_old6', expiresAt: notDue() }); + fetchMock.mockImplementation(async (url: any, init: any) => { + if (String(url).endsWith('/oauth/token')) { + return rotated(6); + } + return init.headers.Authorization === 'Bearer qc_new6' ? json(200, { email: 'a@b.c' }) : json(401, {}); + }); + + const client = await ApiClient.create(); + + expect(await client.getUserInfo()).toEqual({ email: 'a@b.c' }); + expect(tokenCalls()).toHaveLength(1); + expect(fetchMock).toHaveBeenCalledTimes(3); + }); +}); + +describe('refresh token refused', () => { + it('tells the user to run qc login and keeps the credentials', async () => { + writeLogin({ token: 'qc_old7', refreshToken: 'qcr_dead', expiresAt: new Date(Date.now() + DAY).toISOString() }); + const before = readFileSync(credFile, 'utf-8'); + fetchMock.mockResolvedValue(json(400, { error: 'invalid_grant', error_description: 'Refresh token revoked' })); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const auth = await ctx.getEffectiveAuth(); + await ctx.getEffectiveAuth(); + + expect(auth?.token).toBe('qc_old7'); + expect(tokenCalls()).toHaveLength(1); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0][0])).toContain('Refresh token revoked'); + expect(String(warn.mock.calls[0][0])).toContain('qc login'); + expect(readFileSync(credFile, 'utf-8')).toBe(before); + }); +}); diff --git a/__tests__/utils/token-refresh.test.ts b/__tests__/utils/token-refresh.test.ts new file mode 100644 index 0000000..2bcdc8f --- /dev/null +++ b/__tests__/utils/token-refresh.test.ts @@ -0,0 +1,134 @@ +import { describe, it, expect, jest, beforeEach, afterEach, afterAll } from '@jest/globals'; +import { mkdtempSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { forceRefresh, isRefreshDue, refreshIfDue, toAuthUpdates, REFRESH_WINDOW_MS } from '../../src/utils/token-refresh.js'; +import type { AuthConfig } from '../../src/types/auth.js'; + +const DAY = 24 * 60 * 60 * 1000; +const dir = mkdtempSync(join(tmpdir(), 'qc-refresh-unit-')); +const lockFile = join(dir, 'credentials.lock'); +afterAll(() => rmSync(dir, { recursive: true, force: true })); +const HOST = 'https://dashboard.quantcdn.io'; + +const login = (msFromNow: number, extra: Partial = {}): AuthConfig => ({ + token: 'qc_old', + refreshToken: 'qcr_old', + expiresAt: new Date(Date.now() + msFromNow).toISOString(), + host: HOST, + ...extra, +}); + +const jsonResponse = (status: number, body: unknown) => ({ + ok: status >= 200 && status < 300, + status, + json: async () => body, +}); + +describe('isRefreshDue', () => { + it('never refreshes a legacy login without refresh token or expiry', () => { + expect(isRefreshDue({ token: 't', host: HOST })).toBe(false); + expect(isRefreshDue({ token: 't', host: HOST, refreshToken: 'r' })).toBe(false); + expect(isRefreshDue({ token: 't', host: HOST, expiresAt: new Date().toISOString() })).toBe(false); + }); + + it('refreshes inside the window and after expiry, not before', () => { + expect(isRefreshDue(login(30 * DAY))).toBe(false); + expect(isRefreshDue(login(REFRESH_WINDOW_MS - DAY))).toBe(true); + expect(isRefreshDue(login(-DAY))).toBe(true); + }); +}); + +describe('toAuthUpdates', () => { + it('maps the token response and leaves expiry unset without expires_in', () => { + const now = Date.UTC(2026, 0, 1); + expect(toAuthUpdates({ access_token: 'a', refresh_token: 'r', expires_in: 60, token_type: 'Bearer' }, now)) + .toEqual({ token: 'a', refreshToken: 'r', expiresAt: new Date(now + 60000).toISOString() }); + expect(toAuthUpdates({ access_token: 'a', token_type: 'Bearer' }, now).expiresAt).toBeUndefined(); + }); + + it('leaves refreshToken out when the response has no refresh_token, so the stored one is kept', () => { + const updates = toAuthUpdates({ access_token: 'a', expires_in: 60, token_type: 'Bearer' }); + expect('refreshToken' in updates).toBe(false); + expect({ ...login(DAY), ...updates }.refreshToken).toBe('qcr_old'); + }); +}); + +describe('refreshIfDue', () => { + const fetchMock = jest.fn<(...args: any[]) => any>(); + const save = jest.fn<(...args: any[]) => any>(); + const reload = jest.fn<(...args: any[]) => any>(); + const realFetch = global.fetch; + + beforeEach(() => { + jest.clearAllMocks(); + global.fetch = fetchMock as any; + save.mockResolvedValue(undefined); + reload.mockImplementation(async () => login(DAY)); + }); + + afterEach(() => { + global.fetch = realFetch; + }); + + it('does not call the portal when no refresh is due', async () => { + const auth = login(30 * DAY); + expect(await refreshIfDue(auth, { save, reload, lockFile })).toBe(auth); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('never refreshes a legacy login, even after a 401', async () => { + const legacy: AuthConfig = { token: 'legacy', host: HOST }; + expect(await refreshIfDue(legacy, { save, reload, lockFile })).toBe(legacy); + expect(await forceRefresh(legacy, { save, reload, lockFile })).toBeNull(); + expect(fetchMock).not.toHaveBeenCalled(); + expect(reload).not.toHaveBeenCalled(); + }); + + it('exchanges the refresh token and saves the new pair', async () => { + fetchMock.mockResolvedValue(jsonResponse(200, { + access_token: 'qc_new', refresh_token: 'qcr_new', expires_in: 90 * 86400, token_type: 'Bearer', + })); + + const result = await refreshIfDue(login(DAY), { save, reload, lockFile }); + + expect(result.token).toBe('qc_new'); + expect(result.refreshToken).toBe('qcr_new'); + expect(save).toHaveBeenCalledWith(expect.objectContaining({ token: 'qc_new', refreshToken: 'qcr_new' })); + const [url, init] = fetchMock.mock.calls[0] as [string, any]; + expect(url).toBe(`${HOST}/oauth/token`); + const body = new URLSearchParams(init.body); + expect(body.get('grant_type')).toBe('refresh_token'); + expect(body.get('client_id')).toBe('quant-cli'); + expect(body.get('refresh_token')).toBe('qcr_old'); + }); + + it('uses the credentials another process saved, and sends no request', async () => { + reload.mockResolvedValue(login(90 * DAY, { token: 'qc_peer', refreshToken: 'qcr_peer' })); + + const result = await refreshIfDue(login(DAY), { save, reload, lockFile }); + + expect(result.token).toBe('qc_peer'); + expect(fetchMock).not.toHaveBeenCalled(); + expect(save).not.toHaveBeenCalled(); + }); + + it('force-refreshes a token that is not yet due', async () => { + reload.mockResolvedValue(login(60 * DAY)); + fetchMock.mockResolvedValue(jsonResponse(200, { access_token: 'qc_new', refresh_token: 'qcr_new', expires_in: 60, token_type: 'Bearer' })); + + const result = await forceRefresh(login(60 * DAY), { save, reload, lockFile }); + + expect(result?.token).toBe('qc_new'); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('keeps the stored login when the refresh fails', async () => { + fetchMock.mockRejectedValue(new Error('network down')); + const auth = login(DAY); + reload.mockResolvedValue(auth); + + expect(await refreshIfDue(auth, { save, reload, lockFile })).toBe(auth); + expect(save).not.toHaveBeenCalled(); + }); +}); diff --git a/src/utils/api.ts b/src/utils/api.ts index cb74587..c8324c5 100644 --- a/src/utils/api.ts +++ b/src/utils/api.ts @@ -1,5 +1,6 @@ -import { resolveEffectiveContext, ContextOverrides, EffectiveContext } from './context.js'; +import { resolveEffectiveContext, ContextOverrides, EffectiveContext, TokenRenewer } from './context.js'; import { Logger } from './logger.js'; +import { createRenewingAxios } from './auth-retry.js'; import { ApplicationsApi, EnvironmentsApi, SSHAccessApi, BackupManagementApi, ProjectsApi, CrawlersApi, CommandsApi, VariablesApi, SecretsApi, Configuration } from '@quantcdn/quant-client'; const logger = new Logger('API'); @@ -38,27 +39,31 @@ export class ApiClient { private defaultApplicationId?: string; private defaultEnvironmentId?: string; private token: string; + private renewToken?: TokenRenewer; - constructor(baseUrl: string, token: string, defaultOrganizationId?: string, defaultApplicationId?: string, defaultEnvironmentId?: string) { - // Configure the APIs using Configuration object (v4.3.0) - // The client automatically handles Authorization header via accessToken + constructor(baseUrl: string, token: string, defaultOrganizationId?: string, defaultApplicationId?: string, defaultEnvironmentId?: string, renewToken?: TokenRenewer) { + this.token = token; + this.renewToken = renewToken; + + // The client reads accessToken for every request, so a renewed token is used at once. const config = new Configuration({ basePath: baseUrl, - accessToken: token + accessToken: () => this.token }); + // Legacy logins (no refresh token) keep the default axios and never retry. + const http = renewToken ? createRenewingAxios((sent) => this.renew(sent)) : undefined; - this.applicationsApi = new ApplicationsApi(config); - this.environmentsApi = new EnvironmentsApi(config); - this.sshAccessApi = new SSHAccessApi(config); - this.backupManagementApi = new BackupManagementApi(config); - this.projectsApi = new ProjectsApi(config); - this.crawlersApi = new CrawlersApi(config); - this.commandsApi = new CommandsApi(config); - this.variablesApi = new VariablesApi(config); - this.secretsApi = new SecretsApi(config); + this.applicationsApi = new ApplicationsApi(config, undefined, http); + this.environmentsApi = new EnvironmentsApi(config, undefined, http); + this.sshAccessApi = new SSHAccessApi(config, undefined, http); + this.backupManagementApi = new BackupManagementApi(config, undefined, http); + this.projectsApi = new ProjectsApi(config, undefined, http); + this.crawlersApi = new CrawlersApi(config, undefined, http); + this.commandsApi = new CommandsApi(config, undefined, http); + this.variablesApi = new VariablesApi(config, undefined, http); + this.secretsApi = new SecretsApi(config, undefined, http); this.baseUrl = baseUrl; - this.token = token; this.defaultOrganizationId = defaultOrganizationId; this.defaultApplicationId = defaultApplicationId; this.defaultEnvironmentId = defaultEnvironmentId; @@ -72,10 +77,23 @@ export class ApiClient { context.token, context.activeOrganization, context.activeApplication, - context.activeEnvironment + context.activeEnvironment, + context.renewToken ); } + /** Renews the token after a 401 for sentToken. Returns the new token, or null. */ + private async renew(sentToken: string): Promise { + if (!this.renewToken) { + return null; + } + const token = await this.renewToken(sentToken); + if (token) { + this.token = token; + } + return token; + } + // Old request method removed - now using TypeScript client // Using TypeScript client methods directly instead of manual HTTP calls @@ -142,16 +160,31 @@ export class ApiClient { throw new Error('getOrganizations not yet implemented - use getUserInfo instead'); } + private fetchUserInfo(token: string): Promise { + return fetch(`${this.baseUrl}/api/oauth/user`, { + headers: { + 'Authorization': `Bearer ${token}`, + 'Accept': 'application/json' + } + }); + } + + /** fetchUserInfo, renewed and sent again once after a 401. */ + private async fetchUserInfoWithRenewal(): Promise { + const sent = this.token; + const response = await this.fetchUserInfo(sent); + if (response.status !== 401) { + return response; + } + const token = await this.renew(sent); + return token ? this.fetchUserInfo(token) : response; + } + async getUserInfo(): Promise { // OAuth user info is fetched directly, not through the TypeScript client // This endpoint is not part of the v3 API, it's part of the OAuth system try { - const response = await fetch(`${this.baseUrl}/api/oauth/user`, { - headers: { - 'Authorization': `Bearer ${this.token}`, - 'Accept': 'application/json' - } - }); + const response = await this.fetchUserInfoWithRenewal(); if (!response.ok) { if (response.status === 401) { diff --git a/src/utils/auth-retry.ts b/src/utils/auth-retry.ts new file mode 100644 index 0000000..664f682 --- /dev/null +++ b/src/utils/auth-retry.ts @@ -0,0 +1,34 @@ +import axios, { AxiosError, AxiosInstance, InternalAxiosRequestConfig } from 'axios'; + +/** Called with the token a request sent. Returns a new token, or null. */ +export type Renew = (sentToken: string) => Promise; + +type RetryConfig = InternalAxiosRequestConfig & { _qcRenewed?: boolean }; + +function sentToken(config: RetryConfig): string { + return String(config.headers?.Authorization ?? '').replace(/^Bearer\s+/i, ''); +} + +async function retryAfterRenewal(http: AxiosInstance, renew: Renew, error: AxiosError): Promise { + const config = error?.config as RetryConfig | undefined; + if (error?.response?.status !== 401 || !config || config._qcRenewed) { + throw error; + } + config._qcRenewed = true; + const token = await renew(sentToken(config)); + if (!token) { + throw error; + } + config.headers.set('Authorization', `Bearer ${token}`); + return http.request(config); +} + +/** + * An axios instance for the quant-client APIs. After a 401 it renews the token + * once and sends the same request again. A second 401 goes to the caller. + */ +export function createRenewingAxios(renew: Renew): AxiosInstance { + const http = axios.create(); + http.interceptors.response.use(undefined, (error) => retryAfterRenewal(http, renew, error)); + return http; +} diff --git a/src/utils/config.ts b/src/utils/config.ts index e6cf3be..2d6d76c 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -1,10 +1,14 @@ import { promises as fs } from 'fs'; import { homedir } from 'os'; -import { join, resolve } from 'path'; +import { dirname, join, resolve } from 'path'; +import { randomBytes } from 'crypto'; import { AuthConfig, MultiPlatformConfig, PlatformInfo } from '../types/auth.js'; +import { withFileLock } from './file-lock.js'; const CONFIG_DIR = process.env.QUANT_CONFIG_DIR || join(homedir(), '.quant'); export const CONFIG_FILE = join(CONFIG_DIR, 'credentials'); +/** Every qc process holds this lock while it reads and then rewrites CONFIG_FILE. */ +export const CREDENTIALS_LOCK_FILE = `${CONFIG_FILE}.lock`; export const VRT_CONFIG_FILE = join(CONFIG_DIR, 'vrt-config.json'); export interface VRTProjectConfig { @@ -34,9 +38,30 @@ export async function ensureConfigDir(): Promise { } } -export async function saveAuthConfig(config: AuthConfig): Promise { +/** + * Replaces the credentials file in one step: a reader sees the old file or the + * new file, never a partial write. The file is readable by its owner only. + */ +async function writeCredentialsFile(content: string): Promise { + await ensureConfigDir(); + const temp = join(dirname(CONFIG_FILE), `.credentials.${process.pid}.${randomBytes(6).toString('hex')}.tmp`); + try { + await fs.writeFile(temp, content, { encoding: 'utf-8', mode: 0o600 }); + await fs.rename(temp, CONFIG_FILE); + } catch (error) { + await fs.unlink(temp).catch(() => undefined); + throw error; + } +} + +/** Runs a read-modify-write of the credentials file under the credentials lock. */ +export async function withCredentialsLock(fn: () => Promise): Promise { await ensureConfigDir(); - await fs.writeFile(CONFIG_FILE, JSON.stringify(config, null, 2), 'utf-8'); + return withFileLock(CREDENTIALS_LOCK_FILE, fn); +} + +export async function saveAuthConfig(config: AuthConfig): Promise { + await writeCredentialsFile(JSON.stringify(config, null, 2)); } export async function loadAuthConfig(): Promise { @@ -76,8 +101,7 @@ export async function loadMultiPlatformConfig(): Promise { } export async function saveMultiPlatformConfig(config: MultiPlatformConfig): Promise { - await ensureConfigDir(); - await fs.writeFile(CONFIG_FILE, JSON.stringify(config, null, 2), 'utf-8'); + await writeCredentialsFile(JSON.stringify(config, null, 2)); } export async function getActivePlatformConfig(): Promise { @@ -98,6 +122,10 @@ export async function getActivePlatformConfig(): Promise { } export async function savePlatformConfig(platformId: string, authConfig: AuthConfig, platformInfo: PlatformInfo): Promise { + await withCredentialsLock(() => applyPlatformConfig(platformId, authConfig, platformInfo)); +} + +async function applyPlatformConfig(platformId: string, authConfig: AuthConfig, platformInfo: PlatformInfo): Promise { const multiConfig = await loadMultiPlatformConfig(); multiConfig.platforms[platformId] = { @@ -114,6 +142,10 @@ export async function savePlatformConfig(platformId: string, authConfig: AuthCon } export async function switchPlatform(platformId: string): Promise { + return withCredentialsLock(() => applySwitchPlatform(platformId)); +} + +async function applySwitchPlatform(platformId: string): Promise { const multiConfig = await loadMultiPlatformConfig(); if (!multiConfig.platforms[platformId]) { @@ -136,6 +168,10 @@ export async function listPlatforms(): Promise { + return withCredentialsLock(() => applyRemovePlatform(platformId)); +} + +async function applyRemovePlatform(platformId: string): Promise { const multiConfig = await loadMultiPlatformConfig(); if (!multiConfig.platforms[platformId]) { @@ -203,6 +239,11 @@ function generatePlatformInfo(host: string): PlatformInfo { // Helper to save updates to a named platform export async function savePlatformUpdates(platformId: string, updates: Partial): Promise { + await withCredentialsLock(() => applyPlatformUpdates(platformId, updates)); +} + +/** savePlatformUpdates for a caller that already holds the credentials lock. */ +export async function applyPlatformUpdates(platformId: string, updates: Partial): Promise { const multiConfig = await loadMultiPlatformConfig(); const current = multiConfig.platforms[platformId]; if (!current) { diff --git a/src/utils/context.ts b/src/utils/context.ts index c07faa0..8ce6095 100644 --- a/src/utils/context.ts +++ b/src/utils/context.ts @@ -1,7 +1,8 @@ -import { getActivePlatformConfig, loadMultiPlatformConfig, saveActivePlatformConfig, savePlatformUpdates } from './config.js'; +import { applyPlatformUpdates, CREDENTIALS_LOCK_FILE, loadMultiPlatformConfig, saveActivePlatformConfig, savePlatformUpdates } from './config.js'; import type { AuthConfig } from '../types/auth.js'; import { getProjectConfig, ProjectConfig } from './project-config.js'; import { Logger } from './logger.js'; +import { forceRefresh, refreshIfDue, RefreshStore } from './token-refresh.js'; const logger = new Logger('Context'); @@ -21,21 +22,62 @@ export interface EffectiveContext { activeApplication?: string; activeEnvironment?: string; organizations?: any[]; + /** Present only for logins with a refresh token. See tokenRenewer(). */ + renewToken?: TokenRenewer; } /** - * Loads the stored credentials for a named platform without changing the persisted active platform. + * Called after the portal answers 401 for sentToken. Returns a new access token, + * or null when the login cannot be renewed. */ -async function loadPlatformAuth(platformId: string): Promise { +export type TokenRenewer = (sentToken: string) => Promise; + +/** The stored login for a platform, or null when there is none. */ +async function loadStoredAuth(platformId: string): Promise { const multiConfig = await loadMultiPlatformConfig(); const stored = multiConfig.platforms[platformId]; - if (!stored || !stored.token) { - throw new Error(`Not logged in to platform ${platformId}. Run qc login and choose ${platformId}, or qc platform list.`); + if (!stored) { + return null; } const { platformInfo, ...auth } = stored; return auth; } +/** + * Loads the stored credentials for a named platform without changing the persisted active platform. + */ +async function loadPlatformAuth(platformId: string): Promise { + const auth = await loadStoredAuth(platformId); + if (!auth || !auth.token) { + throw new Error(`Not logged in to platform ${platformId}. Run qc login and choose ${platformId}, or qc platform list.`); + } + return auth; +} + +/** + * The refresh store for one platform. It is bound to the platform id, so a + * refresh never writes to another platform, even if the active one changes. + */ +export function platformRefreshStore(platformId: string): RefreshStore { + return { + reload: () => loadStoredAuth(platformId), + save: (updates) => applyPlatformUpdates(platformId, updates), + lockFile: CREDENTIALS_LOCK_FILE, + }; +} + +/** A TokenRenewer for this login, or undefined for a legacy login without a refresh token. */ +export function tokenRenewer(auth: AuthConfig, platformId: string): TokenRenewer | undefined { + if (!auth.refreshToken) { + return undefined; + } + const store = platformRefreshStore(platformId); + return async (sentToken) => { + const renewed = await forceRefresh({ ...auth, token: sentToken }, store); + return renewed?.token ?? null; + }; +} + function describe(values: Record): string { return Object.entries(values) .filter(([, v]) => v) @@ -68,8 +110,22 @@ export function requestedPlatform(flag?: string): string | undefined { * token and stored org/app context always come from the same platform. */ export async function getEffectiveAuth(platformFlag?: string): Promise { - const id = requestedPlatform(platformFlag); - return id ? loadPlatformAuth(id) : getActivePlatformConfig(); + const effective = await loadEffectiveAuth(platformFlag); + return effective ? effective.auth : null; +} + +/** getEffectiveAuth, plus the id of the platform the login came from. */ +async function loadEffectiveAuth(platformFlag?: string): Promise<{ platformId: string; auth: AuthConfig } | null> { + const requested = requestedPlatform(platformFlag); + const platformId = requested || (await loadMultiPlatformConfig()).activePlatform; + if (!platformId) { + return null; + } + const auth = requested ? await loadPlatformAuth(platformId) : await loadStoredAuth(platformId); + if (!auth) { + return null; + } + return { platformId, auth: await refreshIfDue(auth, platformRefreshStore(platformId)) }; } /** @@ -93,10 +149,11 @@ export async function resolveEffectiveContext(overrides: ContextOverrides = {}): const projectConfig = getProjectConfig(); // Platform priority: CLI flag > .quant.yml > active platform. Never persisted. - const auth = await getEffectiveAuth(overrides.platform); - if (!auth || !auth.token) { + const effective = await loadEffectiveAuth(overrides.platform); + if (!effective || !effective.auth.token) { throw new Error('Not authenticated. Run `quant-cloud login` to authenticate.'); } + const { auth, platformId } = effective; // Apply in priority order: CLI flags > project config > stored config const effectiveContext: EffectiveContext = { @@ -108,6 +165,7 @@ export async function resolveEffectiveContext(overrides: ContextOverrides = {}): activeOrganization: overrides.org || projectConfig.org || auth.activeOrganization, activeApplication: overrides.app || projectConfig.app || auth.activeApplication, activeEnvironment: overrides.env || projectConfig.env || auth.activeEnvironment, + renewToken: tokenRenewer(auth, platformId), }; logContextSources(overrides, projectConfig, effectiveContext); diff --git a/src/utils/file-lock.ts b/src/utils/file-lock.ts new file mode 100644 index 0000000..6085425 --- /dev/null +++ b/src/utils/file-lock.ts @@ -0,0 +1,66 @@ +import { promises as fs } from 'fs'; + +/** A lock older than this belongs to a process that died, and is taken over. */ +export const LOCK_STALE_MS = 30_000; + +const POLL_MS = 100; +const MAX_WAIT_MS = 2 * LOCK_STALE_MS; + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +function errorCode(error: unknown): string | undefined { + return (error as NodeJS.ErrnoException)?.code; +} + +/** Removes the lock file if its owner has held it for longer than LOCK_STALE_MS. */ +async function removeIfStale(lockFile: string): Promise { + try { + const { mtimeMs } = await fs.stat(lockFile); + if (Date.now() - mtimeMs > LOCK_STALE_MS) { + await fs.unlink(lockFile); + } + } catch (error) { + if (errorCode(error) !== 'ENOENT') { + throw error; + } + } +} + +/** Creates the lock file. Returns false when another process holds it. */ +async function tryAcquire(lockFile: string): Promise { + try { + const handle = await fs.open(lockFile, 'wx', 0o600); + await handle.writeFile(String(process.pid)); + await handle.close(); + return true; + } catch (error) { + if (errorCode(error) === 'EEXIST') { + return false; + } + throw error; + } +} + +async function acquire(lockFile: string): Promise { + const deadline = Date.now() + MAX_WAIT_MS; + while (!(await tryAcquire(lockFile))) { + if (Date.now() > deadline) { + throw new Error(`Timed out waiting for ${lockFile}. If no other qc command is running, delete that file.`); + } + await removeIfStale(lockFile); + await sleep(POLL_MS); + } +} + +/** + * Runs fn while this process holds an exclusive lock file. Every qc process that + * uses the same lock file waits for the others, in this process and in others. + */ +export async function withFileLock(lockFile: string, fn: () => Promise): Promise { + await acquire(lockFile); + try { + return await fn(); + } finally { + await fs.unlink(lockFile).catch(() => undefined); + } +} diff --git a/src/utils/token-refresh.ts b/src/utils/token-refresh.ts new file mode 100644 index 0000000..cd0aef9 --- /dev/null +++ b/src/utils/token-refresh.ts @@ -0,0 +1,182 @@ +import type { AuthConfig, LoginResponse } from '../types/auth.js'; +import { withFileLock } from './file-lock.js'; +import { Logger } from './logger.js'; + +const logger = new Logger('Auth'); + +const CLIENT_ID = 'quant-cli'; + +/** Refresh when the access token has this much life or less left. */ +export const REFRESH_WINDOW_MS = 7 * 24 * 60 * 60 * 1000; + +/** + * The refresh request must end well before LOCK_STALE_MS, or a waiting process + * takes the lock and sends the same refresh token, which revokes the chain. + */ +const REFRESH_TIMEOUT_MS = 15_000; + +/** Refresh tokens the portal refused in this process. They are not sent again. */ +const rejectedTokens = new Set(); + +/** Where refreshed credentials go, and how to read what another qc process saved. */ +export interface RefreshStore { + /** Saves the refreshed tokens. Called while the lock is held. */ + save(updates: Partial): Promise; + /** Reads the stored login again. */ + reload(): Promise; + /** The lock file that every qc process holds while it refreshes or saves. */ + lockFile: string; +} + +/** The portal refused the refresh token (HTTP 400 invalid_grant or invalid_client). */ +export class TokenRefreshError extends Error { + constructor(public readonly code: string, description?: string) { + super(description || code); + this.name = 'TokenRefreshError'; + } +} + +/** + * True when the stored login can and should be refreshed. Legacy logins have no + * refresh token or expiry and are never refreshed. + */ +export function isRefreshDue(auth: AuthConfig, now: number = Date.now()): boolean { + if (!auth.refreshToken || !auth.expiresAt) { + return false; + } + const expiry = Date.parse(auth.expiresAt); + return Number.isNaN(expiry) || expiry - now <= REFRESH_WINDOW_MS; +} + +/** + * The credential fields to store from an /oauth/token response. A response + * without refresh_token keeps the stored refresh token: the key is left out. + */ +export function toAuthUpdates(body: LoginResponse, now: number = Date.now()): Partial { + const updates: Partial = { + token: body.access_token, + expiresAt: typeof body.expires_in === 'number' + ? new Date(now + body.expires_in * 1000).toISOString() + : undefined, + }; + if (body.refresh_token) { + updates.refreshToken = body.refresh_token; + } + return updates; +} + +interface TokenErrorBody { + error?: string; + error_description?: string; +} + +function refreshFailure(status: number, body: TokenErrorBody): Error { + if (body.error) { + return new TokenRefreshError(body.error, body.error_description); + } + return new Error(`HTTP ${status}`); +} + +async function requestRefresh(auth: AuthConfig): Promise> { + const refreshToken = auth.refreshToken as string; + if (rejectedTokens.has(refreshToken)) { + throw new TokenRefreshError('invalid_grant', 'Refresh token was refused earlier'); + } + const response = await fetch(new URL('/oauth/token', auth.host).toString(), { + method: 'POST', + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + 'Accept': 'application/json', + }, + body: new URLSearchParams({ + grant_type: 'refresh_token', + client_id: CLIENT_ID, + refresh_token: refreshToken, + }).toString(), + signal: AbortSignal.timeout(REFRESH_TIMEOUT_MS), + }); + + const body = await response.json().catch(() => ({})) as Partial & TokenErrorBody; + if (!response.ok || typeof body.access_token !== 'string') { + const failure = refreshFailure(response.status, body); + if (failure instanceof TokenRefreshError) { + rejectedTokens.add(refreshToken); + } + throw failure; + } + return toAuthUpdates(body as LoginResponse); +} + +/** + * Refreshes under the lock. The stored credentials are read again first: when + * another qc process already rotated the token, its result is used and no + * request is sent, so one refresh token is never sent twice. + */ +async function refreshLocked( + auth: AuthConfig, + store: RefreshStore, + shouldRefresh: (latest: AuthConfig) => boolean, +): Promise { + return withFileLock(store.lockFile, async () => { + const latest = await store.reload(); + if (!latest || latest.token !== auth.token || !shouldRefresh(latest)) { + return latest ?? auth; + } + const updates = await requestRefresh(latest); + await store.save(updates); + logger.debug('Access token refreshed'); + return { ...latest, ...updates }; + }); +} + +let warnedRejected = false; + +function reportFailure(auth: AuthConfig, error: unknown): void { + const reason = error instanceof Error ? error.message : String(error); + if (error instanceof TokenRefreshError) { + if (warnedRejected) { + return; + } + warnedRejected = true; + logger.warn(`Your Quant login could not be renewed (${reason}). Run \`qc login\` to sign in again.`); + return; + } + logger.debug(`Token refresh failed: ${reason}`); + if (auth.expiresAt && Date.parse(auth.expiresAt) <= Date.now()) { + logger.warn(`Your Quant login expired and could not be renewed (${reason}). Run \`qc login\` to sign in again.`); + } +} + +/** + * Returns the login to use for this command. When the access token is close to + * expiry, it is exchanged for a new pair, which is saved. On failure the stored + * login is returned unchanged: it is still valid if it has not expired. + */ +export async function refreshIfDue(auth: AuthConfig, store: RefreshStore): Promise { + if (!isRefreshDue(auth)) { + return auth; + } + try { + return await refreshLocked(auth, store, (latest) => isRefreshDue(latest)); + } catch (error) { + reportFailure(auth, error); + return auth; + } +} + +/** + * Refreshes after the portal rejected auth.token with 401, even when the token + * is not yet due. Returns the new login, or null when no new token is available. + */ +export async function forceRefresh(auth: AuthConfig, store: RefreshStore): Promise { + if (!auth.refreshToken) { + return null; + } + try { + const renewed = await refreshLocked(auth, store, (latest) => Boolean(latest.refreshToken)); + return renewed.token !== auth.token ? renewed : null; + } catch (error) { + reportFailure(auth, error); + return null; + } +} From 50da7a2035aacdd1388a1e311ce592c36e8eb3f6 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 20:55:04 +1000 Subject: [PATCH 03/15] fix(login): do not log the authorization code and PKCE verifier With LOG_LEVEL=debug, qc login printed the token request body, which holds the authorization code and the code_verifier. Log only the token URL. --- src/commands/login.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/commands/login.ts b/src/commands/login.ts index 374d7f3..84950d5 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -374,9 +374,8 @@ async function exchangeCodeForToken(code: string, codeVerifier: string, host: st code_verifier: codeVerifier, }); - logger.debug('Token request details:'); - logger.debug(` URL: ${tokenUrl.toString()}`); - logger.debug(` Body: ${body.toString()}`); + // Do not log the body: it holds the authorization code and the PKCE verifier. + logger.debug(`Token request: ${tokenUrl.toString()}`); const response = await fetch(tokenUrl.toString(), { method: 'POST', From 9e3968ffc2053dfd905152e2114c0f7f625014f9 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 20:55:04 +1000 Subject: [PATCH 04/15] feat(whoami): show the remaining login time in days when more than 48 hours are left --- __tests__/utils/whoami-expiry.test.ts | 16 +++++ src/commands/whoami.ts | 87 ++++++++++++++++----------- 2 files changed, 67 insertions(+), 36 deletions(-) create mode 100644 __tests__/utils/whoami-expiry.test.ts diff --git a/__tests__/utils/whoami-expiry.test.ts b/__tests__/utils/whoami-expiry.test.ts new file mode 100644 index 0000000..ce31e7a --- /dev/null +++ b/__tests__/utils/whoami-expiry.test.ts @@ -0,0 +1,16 @@ +import { describe, it, expect } from '@jest/globals'; +import { formatTimeLeft } from '../../src/commands/whoami.js'; + +const HOUR = 60 * 60 * 1000; + +describe('formatTimeLeft', () => { + it('shows days when more than 48 hours are left', () => { + expect(formatTimeLeft(90 * 24 * HOUR)).toBe('90 days'); + expect(formatTimeLeft(49 * HOUR)).toBe('2 days'); + }); + + it('shows hours and minutes for 48 hours or less', () => { + expect(formatTimeLeft(48 * HOUR)).toBe('48h 0m'); + expect(formatTimeLeft(5 * HOUR + 20 * 60 * 1000)).toBe('5h 20m'); + }); +}); diff --git a/src/commands/whoami.ts b/src/commands/whoami.ts index 25ac34e..1e59354 100644 --- a/src/commands/whoami.ts +++ b/src/commands/whoami.ts @@ -1,6 +1,6 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import { resolveEffectiveContext } from '../utils/context.js'; +import { resolveEffectiveContext, EffectiveContext } from '../utils/context.js'; import { Logger } from '../utils/logger.js'; const logger = new Logger('WhoAmI'); @@ -14,6 +14,54 @@ export function whoamiCommand(program: Command) { }); } +const HOUR_MS = 60 * 60 * 1000; +const DAY_MS = 24 * HOUR_MS; + +/** "12 days" when more than 48 hours are left, else "5h 20m". */ +export function formatTimeLeft(ms: number): string { + if (ms > 48 * HOUR_MS) { + return `${Math.floor(ms / DAY_MS)} days`; + } + const hours = Math.floor(ms / HOUR_MS); + const minutes = Math.floor((ms % HOUR_MS) / (60 * 1000)); + return `${hours}h ${minutes}m`; +} + +function printExpiry(expiresAt?: string): void { + if (!expiresAt) { + return; + } + const timeLeft = new Date(expiresAt).getTime() - Date.now(); + if (timeLeft > 0) { + logger.info(`Expires in: ${chalk.green(formatTimeLeft(timeLeft))}`); + } +} + +function printActiveOrganization(ctx: EffectiveContext, organizations: NonNullable): void { + const activeOrg = ctx.activeOrganization && organizations.find(o => o.machine_name === ctx.activeOrganization); + if (activeOrg) { + logger.info(`Active organization: ${chalk.green(activeOrg.name)} (${chalk.gray(activeOrg.roles.map((r: any) => r.display_name).join(', '))})`); + } +} + +function printContext(ctx: EffectiveContext): void { + const organizations = ctx.organizations ?? []; + if (organizations.length === 0) { + return; + } + logger.info(`Organizations: ${chalk.cyan(organizations.length.toString())}`); + printActiveOrganization(ctx, organizations); + if (ctx.activeApplication) { + logger.info(`Active application: ${chalk.cyan(ctx.activeApplication)}`); + } + if (ctx.activeEnvironment) { + logger.info(`Active environment: ${chalk.magenta(ctx.activeEnvironment)}`); + } + if (organizations.length > 1) { + logger.info(`${chalk.gray('Use')} ${chalk.cyan('quant-cloud org list')} ${chalk.gray('to see all organizations')}`); + } +} + async function handleWhoAmI() { try { // Get effective context (includes .quant.yml overrides) @@ -30,41 +78,8 @@ async function handleWhoAmI() { logger.info(`Host: ${chalk.cyan(effectiveContext.host)}`); logger.info(`Token: ${chalk.gray(effectiveContext.token.substring(0, 10) + '...')}`); - if (effectiveContext.expiresAt) { - const expiryDate = new Date(effectiveContext.expiresAt); - const now = new Date(); - const timeLeft = Math.max(0, expiryDate.getTime() - now.getTime()); - const hoursLeft = Math.floor(timeLeft / (1000 * 60 * 60)); - const minutesLeft = Math.floor((timeLeft % (1000 * 60 * 60)) / (1000 * 60)); - - if (timeLeft > 0) { - logger.info(`Expires in: ${chalk.green(`${hoursLeft}h ${minutesLeft}m`)}`); - } - } - - if (effectiveContext.organizations && effectiveContext.organizations.length > 0) { - logger.info(`Organizations: ${chalk.cyan(effectiveContext.organizations.length.toString())}`); - - if (effectiveContext.activeOrganization) { - const activeOrg = effectiveContext.organizations.find(o => o.machine_name === effectiveContext.activeOrganization); - if (activeOrg) { - logger.info(`Active organization: ${chalk.green(activeOrg.name)} (${chalk.gray(activeOrg.roles.map((r: any) => r.display_name).join(', '))})`); - } - } - - if (effectiveContext.activeApplication) { - logger.info(`Active application: ${chalk.cyan(effectiveContext.activeApplication)}`); - } - - if (effectiveContext.activeEnvironment) { - logger.info(`Active environment: ${chalk.magenta(effectiveContext.activeEnvironment)}`); - } - - if (effectiveContext.organizations.length > 1) { - logger.info(`${chalk.gray('Use')} ${chalk.cyan('quant-cloud org list')} ${chalk.gray('to see all organizations')}`); - } - } - + printExpiry(effectiveContext.expiresAt); + printContext(effectiveContext); } catch (error: any) { if (error.message?.includes('Not authenticated')) { logger.info('Not authenticated. Run `quant-cloud login` to authenticate.'); From 5b52f7f212970860ec77abff82702339c0e58863 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 21:05:19 +1000 Subject: [PATCH 05/15] fix(auth): credentials save works with symlinks, bind mounts and locked files - Resolve the real path of the credentials file and write the temp file next to it, so a symlinked file (dotfiles manager) stays a symlink and its target is updated. - Retry the rename on EBUSY, EXDEV, EPERM and EACCES with a short backoff (antivirus and open handles on Windows). If it still fails, write the file in place with mode 0600 (the caller holds the lock). This keeps a single-file bind mount (docker -v ~/.quant/credentials:...) working. - Every read before a save (under the lock) now throws when the file cannot be read or parsed. Before, it got an empty config, and the save dropped every other platform. - The single-platform migration write and qc logout take the lock. --- __tests__/utils/credentials-file.test.ts | 87 +++++++++++++ src/commands/logout.ts | 5 +- src/utils/config.ts | 149 +++++++++++++++++++---- 3 files changed, 213 insertions(+), 28 deletions(-) create mode 100644 __tests__/utils/credentials-file.test.ts diff --git a/__tests__/utils/credentials-file.test.ts b/__tests__/utils/credentials-file.test.ts new file mode 100644 index 0000000..0a5f28c --- /dev/null +++ b/__tests__/utils/credentials-file.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect, jest, beforeAll, afterEach, afterAll } from '@jest/globals'; +import { lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync, promises as fsp } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +// Real credentials file in a temp config dir. +const home = mkdtempSync(join(tmpdir(), 'qc-credentials-')); +const configDir = join(home, '.quant'); +process.env.QUANT_CONFIG_DIR = configDir; +if (!process.env.QUANT_CONFIG_DIR.startsWith(home)) { + throw new Error('refusing to run: QUANT_CONFIG_DIR is not the temp dir'); +} +mkdirSync(configDir); + +let config: typeof import('../../src/utils/config.js'); + +beforeAll(async () => { + config = await import('../../src/utils/config.js'); +}); + +afterEach(() => { + jest.restoreAllMocks(); + rmSync(config.CONFIG_FILE, { force: true }); +}); +afterAll(() => rmSync(home, { recursive: true, force: true })); + +const info = (id: string) => ({ id, name: id, host: `https://${id}.example` }); +const twoPlatforms = () => JSON.stringify({ + activePlatform: 'quantcdn', + platforms: { + quantcdn: { token: 'a', host: 'https://quantcdn.example', platformInfo: info('quantcdn') }, + quantgov: { token: 'b', host: 'https://quantgov.example', platformInfo: info('quantgov') }, + }, +}); + +describe('credentials file save', () => { + it('keeps a symlinked credentials file as a symlink and updates its target', async () => { + const target = join(home, 'dotfiles-credentials'); + writeFileSync(target, twoPlatforms()); + symlinkSync(target, config.CONFIG_FILE); + + await config.savePlatformUpdates('quantcdn', { activeOrganization: 'acme' }); + + expect(lstatSync(config.CONFIG_FILE).isSymbolicLink()).toBe(true); + expect(JSON.parse(readFileSync(target, 'utf-8')).platforms.quantcdn.activeOrganization).toBe('acme'); + expect(statSync(target).mode & 0o777).toBe(0o600); + expect(readdirSync(home).filter((f) => f.endsWith('.tmp'))).toEqual([]); + }); + + it('writes in place when the rename fails with EBUSY (single-file bind mount)', async () => { + writeFileSync(config.CONFIG_FILE, twoPlatforms()); + const rename = jest.spyOn(fsp, 'rename').mockRejectedValue(Object.assign(new Error('busy'), { code: 'EBUSY' })); + + await config.savePlatformUpdates('quantgov', { activeOrganization: 'gov-org' }); + + expect(rename).toHaveBeenCalledTimes(4); + const saved = JSON.parse(readFileSync(config.CONFIG_FILE, 'utf-8')); + expect(saved.platforms.quantgov.activeOrganization).toBe('gov-org'); + expect(saved.platforms.quantcdn.token).toBe('a'); + expect(statSync(config.CONFIG_FILE).mode & 0o777).toBe(0o600); + expect(readdirSync(configDir).filter((f) => f.endsWith('.tmp'))).toEqual([]); + }); + + it('refuses to save when the file cannot be parsed, and leaves it unchanged', async () => { + writeFileSync(config.CONFIG_FILE, '{not json'); + + await expect(config.savePlatformUpdates('quantcdn', { activeOrganization: 'x' })).rejects.toThrow(); + + expect(readFileSync(config.CONFIG_FILE, 'utf-8')).toBe('{not json'); + }); + + it('migrates an old single-platform file under the lock', async () => { + writeFileSync(config.CONFIG_FILE, JSON.stringify({ token: 'old', host: 'https://dashboard.quantcdn.io' })); + + const multi = await config.loadMultiPlatformConfig(); + + expect(multi.activePlatform).toBe('quantcdn'); + expect(JSON.parse(readFileSync(config.CONFIG_FILE, 'utf-8')).platforms.quantcdn.token).toBe('old'); + expect(readdirSync(configDir)).toEqual(['credentials']); + }); + + it('logout removes the file under the lock', async () => { + writeFileSync(config.CONFIG_FILE, twoPlatforms()); + await config.clearAuthConfig(); + expect(readdirSync(configDir)).toEqual([]); + }); +}); diff --git a/src/commands/logout.ts b/src/commands/logout.ts index b02272b..7a59914 100644 --- a/src/commands/logout.ts +++ b/src/commands/logout.ts @@ -1,9 +1,8 @@ import { Command } from 'commander'; import inquirer from 'inquirer'; -import { unlink } from 'fs/promises'; import { existsSync } from 'fs'; import chalk from 'chalk'; -import { CONFIG_FILE } from '../utils/config.js'; +import { CONFIG_FILE, clearAuthConfig } from '../utils/config.js'; import { Logger } from '../utils/logger.js'; const logger = new Logger('Logout'); @@ -41,7 +40,7 @@ async function handleLogout() { } // Remove credentials file - await unlink(CONFIG_FILE); + await clearAuthConfig(); logger.info(`${chalk.green('✓')} Successfully logged out`); logger.info('Run `quant-cloud login` to authenticate again'); diff --git a/src/utils/config.ts b/src/utils/config.ts index 2d6d76c..2ed1cd9 100644 --- a/src/utils/config.ts +++ b/src/utils/config.ts @@ -38,20 +38,83 @@ export async function ensureConfigDir(): Promise { } } +/** Rename errors that a retry or an in-place write can get past. */ +const RENAME_FALLBACK_CODES = new Set(['EBUSY', 'EXDEV', 'EPERM', 'EACCES']); +const RENAME_BACKOFF_MS = [50, 150, 400]; + +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +function errorCode(error: unknown): string | undefined { + return (error as NodeJS.ErrnoException)?.code; +} + +/** The real file behind CONFIG_FILE, so a symlinked file stays a symlink. */ +async function credentialsTarget(): Promise { + try { + return await fs.realpath(CONFIG_FILE); + } catch (error) { + if (errorCode(error) === 'ENOENT') { + return CONFIG_FILE; + } + throw error; + } +} + /** - * Replaces the credentials file in one step: a reader sees the old file or the - * new file, never a partial write. The file is readable by its owner only. + * Renames temp over target. Retries EBUSY, EXDEV, EPERM and EACCES with a short + * backoff (antivirus, open handles on Windows). Returns false when the rename + * still fails with one of those codes; throws any other error. */ -async function writeCredentialsFile(content: string): Promise { - await ensureConfigDir(); - const temp = join(dirname(CONFIG_FILE), `.credentials.${process.pid}.${randomBytes(6).toString('hex')}.tmp`); +async function renameWithRetry(temp: string, target: string): Promise { + for (const delay of [...RENAME_BACKOFF_MS, -1]) { + try { + await fs.rename(temp, target); + return true; + } catch (error) { + if (!RENAME_FALLBACK_CODES.has(errorCode(error) ?? '')) { + throw error; + } + if (delay >= 0) { + await sleep(delay); + } + } + } + return false; +} + +/** Writes temp in the target's directory and renames it. Returns false if the rename cannot replace the target. */ +async function replaceByRename(target: string, content: string): Promise { + const temp = join(dirname(target), `.credentials.${process.pid}.${randomBytes(6).toString('hex')}.tmp`); try { await fs.writeFile(temp, content, { encoding: 'utf-8', mode: 0o600 }); - await fs.rename(temp, CONFIG_FILE); + if (await renameWithRetry(temp, target)) { + return true; + } } catch (error) { await fs.unlink(temp).catch(() => undefined); + if (RENAME_FALLBACK_CODES.has(errorCode(error) ?? '')) { + return false; + } throw error; } + await fs.unlink(temp).catch(() => undefined); + return false; +} + +/** + * Replaces the credentials file, readable by its owner only. Normally this is a + * rename, so a reader sees the old file or the new file. When the rename cannot + * replace the file (a single-file bind mount gives EBUSY), the file is written + * in place; callers hold the credentials lock, so no qc process reads it then. + */ +async function writeCredentialsFile(content: string): Promise { + await ensureConfigDir(); + const target = await credentialsTarget(); + if (await replaceByRename(target, content)) { + return; + } + await fs.writeFile(target, content, { encoding: 'utf-8', mode: 0o600 }); + await fs.chmod(target, 0o600).catch(() => undefined); } /** Runs a read-modify-write of the credentials file under the credentials lock. */ @@ -73,29 +136,65 @@ export async function loadAuthConfig(): Promise { } } +/** Deletes the credentials file under the credentials lock (qc logout). */ export async function clearAuthConfig(): Promise { + await withCredentialsLock(async () => { + try { + await fs.unlink(CONFIG_FILE); + } catch (error) { + if (errorCode(error) !== 'ENOENT') { + throw error; + } + } + }); +} + +/** The parsed credentials file, or null when it does not exist. Other errors are thrown. */ +async function readCredentialsFile(): Promise { try { - await fs.unlink(CONFIG_FILE); - } catch { - // File doesn't exist, which is fine + return JSON.parse(await fs.readFile(CONFIG_FILE, 'utf-8')); + } catch (error) { + if (errorCode(error) === 'ENOENT') { + return null; + } + throw error; } } -// Multi-platform configuration management +function isMultiPlatform(parsed: MultiPlatformConfig | AuthConfig): parsed is MultiPlatformConfig { + return Boolean((parsed as MultiPlatformConfig).platforms); +} + +/** + * Reads the credentials for a caller that holds the credentials lock and saves + * afterwards. A file that cannot be read or parsed throws: an empty result here + * would make the next save drop every platform. An old single-platform file is + * migrated and saved. + */ +export async function loadCredentialsLocked(): Promise { + const parsed = await readCredentialsFile(); + if (!parsed) { + return { platforms: {} }; + } + if (isMultiPlatform(parsed)) { + return parsed; + } + return migrateSinglePlatformConfig(parsed); +} + +// Multi-platform configuration management. Read-only callers get an empty +// config when the file is missing or cannot be read. export async function loadMultiPlatformConfig(): Promise { try { - const data = await fs.readFile(CONFIG_FILE, 'utf-8'); - const parsed = JSON.parse(data); - - // Check if it's already a multi-platform config - if (parsed.platforms) { - return parsed as MultiPlatformConfig; + const parsed = await readCredentialsFile(); + if (!parsed) { + return { platforms: {} }; + } + if (isMultiPlatform(parsed)) { + return parsed; } - - // Migrate single-platform config to multi-platform - return await migrateSinglePlatformConfig(parsed as AuthConfig); + return await withCredentialsLock(loadCredentialsLocked); } catch { - // Return empty multi-platform config return { platforms: {} }; } } @@ -126,7 +225,7 @@ export async function savePlatformConfig(platformId: string, authConfig: AuthCon } async function applyPlatformConfig(platformId: string, authConfig: AuthConfig, platformInfo: PlatformInfo): Promise { - const multiConfig = await loadMultiPlatformConfig(); + const multiConfig = await loadCredentialsLocked(); multiConfig.platforms[platformId] = { ...authConfig, @@ -146,7 +245,7 @@ export async function switchPlatform(platformId: string): Promise { } async function applySwitchPlatform(platformId: string): Promise { - const multiConfig = await loadMultiPlatformConfig(); + const multiConfig = await loadCredentialsLocked(); if (!multiConfig.platforms[platformId]) { return false; @@ -172,7 +271,7 @@ export async function removePlatform(platformId: string): Promise { } async function applyRemovePlatform(platformId: string): Promise { - const multiConfig = await loadMultiPlatformConfig(); + const multiConfig = await loadCredentialsLocked(); if (!multiConfig.platforms[platformId]) { return false; @@ -190,7 +289,7 @@ async function applyRemovePlatform(platformId: string): Promise { return true; } -// Migration helper +// Migration helper. The caller holds the credentials lock. async function migrateSinglePlatformConfig(authConfig: AuthConfig): Promise { const platformInfo: PlatformInfo = generatePlatformInfo(authConfig.host); @@ -244,7 +343,7 @@ export async function savePlatformUpdates(platformId: string, updates: Partial): Promise { - const multiConfig = await loadMultiPlatformConfig(); + const multiConfig = await loadCredentialsLocked(); const current = multiConfig.platforms[platformId]; if (!current) { throw new Error(`Platform configuration not found: ${platformId}`); From d045f70d022804d0fee2889621ab4fd6584f882b Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 21:05:19 +1000 Subject: [PATCH 06/15] fix(auth): lock file ownership, safe stale takeover and exit cleanup - The lock file holds a random nonce and the holder's own timestamp. Staleness (90 s) comes from that timestamp, not from mtime. - A stale lock is moved to a unique name first, checked to be the one that was inspected, and only then removed. If another process replaced it, it is put back. So two waiters cannot both take one stale lock. - Release removes the file only if this holder's nonce is still in it. - HeldLock.verify() lets a holder check ownership before it acts. - While held: process.on('exit') removes the lock if this process owns it, and SIGINT/SIGTERM wait until the locked work is done. - If the record cannot be written, the new lock file is closed and removed. --- __tests__/utils/file-lock.test.ts | 87 +++++++++++++ src/utils/file-lock.ts | 202 ++++++++++++++++++++++++++---- 2 files changed, 266 insertions(+), 23 deletions(-) create mode 100644 __tests__/utils/file-lock.test.ts diff --git a/__tests__/utils/file-lock.test.ts b/__tests__/utils/file-lock.test.ts new file mode 100644 index 0000000..e34ff36 --- /dev/null +++ b/__tests__/utils/file-lock.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect, jest, afterEach, afterAll } from '@jest/globals'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync, promises as fsp } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { withFileLock, LOCK_STALE_MS } from '../../src/utils/file-lock.js'; + +const dir = mkdtempSync(join(tmpdir(), 'qc-file-lock-')); +let n = 0; +const nextLock = () => join(dir, `credentials.${++n}.lock`); +const record = (time: number, nonce = 'other') => JSON.stringify({ nonce, pid: 1, time }); + +afterEach(() => jest.restoreAllMocks()); +afterAll(() => rmSync(dir, { recursive: true, force: true })); + +describe('withFileLock', () => { + it('writes a nonce record while held and removes the file after', async () => { + const lockFile = nextLock(); + const seen = await withFileLock(lockFile, async () => JSON.parse(readFileSync(lockFile, 'utf-8'))); + expect(typeof seen.nonce).toBe('string'); + expect(seen.pid).toBe(process.pid); + expect(existsSync(lockFile)).toBe(false); + }); + + it('runs one holder at a time', async () => { + const lockFile = nextLock(); + const events: string[] = []; + const work = (name: string) => withFileLock(lockFile, async () => { + events.push(`${name}-start`); + await new Promise((resolve) => setTimeout(resolve, 50)); + events.push(`${name}-end`); + }); + await Promise.all([work('a'), work('b')]); + expect(events).toEqual(['a-start', 'a-end', 'b-start', 'b-end']); + }); + + it('takes over a lock whose holder timestamp is stale', async () => { + const lockFile = nextLock(); + writeFileSync(lockFile, record(Date.now() - LOCK_STALE_MS - 1000)); + expect(await withFileLock(lockFile, async () => 'ran')).toBe('ran'); + expect(existsSync(lockFile)).toBe(false); + }); + + it('waits for a lock whose holder timestamp is fresh, even with an old mtime', async () => { + const lockFile = nextLock(); + writeFileSync(lockFile, record(Date.now())); + const old = new Date(Date.now() - 10 * LOCK_STALE_MS); + await fsp.utimes(lockFile, old, old); + let ran = false; + const pending = withFileLock(lockFile, async () => { ran = true; }); + await new Promise((resolve) => setTimeout(resolve, 400)); + expect(ran).toBe(false); + rmSync(lockFile); + await pending; + expect(ran).toBe(true); + }); + + it('does not delete a lock that another process now owns, and verify() fails', async () => { + const lockFile = nextLock(); + const taken = record(Date.now(), 'someone-else'); + let verifyError: unknown; + await withFileLock(lockFile, async (lock) => { + writeFileSync(lockFile, taken); + verifyError = await lock.verify().catch((error) => error); + }); + expect(verifyError).toBeInstanceOf(Error); + expect(readFileSync(lockFile, 'utf-8')).toBe(taken); + }); + + it('removes the lock file when the record cannot be written', async () => { + const lockFile = nextLock(); + const realOpen = fsp.open.bind(fsp); + jest.spyOn(fsp, 'open').mockImplementationOnce(async (...args: any[]) => { + const handle: any = await (realOpen as any)(...args); + handle.writeFile = async () => { throw new Error('disk full'); }; + return handle; + }); + await expect(withFileLock(lockFile, async () => 'ran')).rejects.toThrow('disk full'); + expect(existsSync(lockFile)).toBe(false); + }); + + it('adds an exit handler while held and removes it on release', async () => { + const before = process.listenerCount('exit'); + const during = await withFileLock(nextLock(), async () => process.listenerCount('exit')); + expect(during).toBe(before + 1); + expect(process.listenerCount('exit')).toBe(before); + }); +}); diff --git a/src/utils/file-lock.ts b/src/utils/file-lock.ts index 6085425..ee385f7 100644 --- a/src/utils/file-lock.ts +++ b/src/utils/file-lock.ts @@ -1,10 +1,28 @@ -import { promises as fs } from 'fs'; +import { promises as fs, readFileSync, unlinkSync } from 'fs'; +import { randomBytes } from 'crypto'; -/** A lock older than this belongs to a process that died, and is taken over. */ -export const LOCK_STALE_MS = 30_000; +/** + * A lock older than this (by the holder's own timestamp) belongs to a process + * that died, and is taken over. It must be longer than the refresh timeout. + */ +export const LOCK_STALE_MS = 90_000; const POLL_MS = 100; const MAX_WAIT_MS = 2 * LOCK_STALE_MS; +const DEFERRED_SIGNALS: NodeJS.Signals[] = ['SIGINT', 'SIGTERM']; + +/** What a holder writes into the lock file. `time` is the holder's own clock. */ +interface LockRecord { + nonce: string; + pid: number; + time: number; +} + +/** Passed to the locked function. */ +export interface HeldLock { + /** Throws when another process has taken the lock over. */ + verify(): Promise; +} const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); @@ -12,38 +30,118 @@ function errorCode(error: unknown): string | undefined { return (error as NodeJS.ErrnoException)?.code; } -/** Removes the lock file if its owner has held it for longer than LOCK_STALE_MS. */ -async function removeIfStale(lockFile: string): Promise { +function parseRecord(content: string): LockRecord | null { + try { + const record = JSON.parse(content) as LockRecord; + return typeof record.nonce === 'string' && typeof record.time === 'number' ? record : null; + } catch { + return null; + } +} + +/** The lock file content, or null when there is no lock file. */ +async function readLock(lockFile: string): Promise { try { - const { mtimeMs } = await fs.stat(lockFile); - if (Date.now() - mtimeMs > LOCK_STALE_MS) { - await fs.unlink(lockFile); + return await fs.readFile(lockFile, 'utf-8'); + } catch (error) { + if (errorCode(error) === 'ENOENT') { + return null; } + throw error; + } +} + +/** + * Staleness comes from the holder's timestamp in the file, not from mtime. A file + * without a valid record (a holder between create and write) uses its mtime. + */ +async function isStale(lockFile: string, content: string): Promise { + const record = parseRecord(content); + if (record) { + return Date.now() - record.time > LOCK_STALE_MS; + } + const { mtimeMs } = await fs.stat(lockFile); + return Date.now() - mtimeMs > LOCK_STALE_MS; +} + +/** Puts a lock file that was moved aside back, unless a new lock exists now. */ +async function restoreLock(aside: string, lockFile: string): Promise { + try { + await fs.link(aside, lockFile); + await fs.unlink(aside); + } catch (error) { + if (errorCode(error) === 'EEXIST') { + await fs.unlink(aside).catch(() => undefined); + return; + } + await fs.rename(aside, lockFile).catch(() => undefined); + } +} + +/** + * Removes a stale lock. The file is first moved to a unique name, so only one + * process can take it. If the moved file is not the stale lock that was + * inspected (another process replaced it), it is put back. + */ +async function breakStaleLock(lockFile: string, inspected: string): Promise { + const aside = `${lockFile}.stale.${randomBytes(6).toString('hex')}`; + try { + await fs.rename(lockFile, aside); } catch (error) { - if (errorCode(error) !== 'ENOENT') { - throw error; + if (errorCode(error) === 'ENOENT') { + return; } + throw error; + } + if ((await readLock(aside)) === inspected) { + await fs.unlink(aside).catch(() => undefined); + return; + } + await restoreLock(aside, lockFile); +} + +async function removeIfStale(lockFile: string): Promise { + const content = await readLock(lockFile); + if (content !== null && (await isStale(lockFile, content))) { + await breakStaleLock(lockFile, content); + } +} + +/** Writes the record. On failure the half-made lock file is removed. */ +async function writeRecord(handle: fs.FileHandle, lockFile: string, record: LockRecord): Promise { + try { + await handle.writeFile(JSON.stringify(record)); + } catch (error) { + await handle.close().catch(() => undefined); + await fs.unlink(lockFile).catch(() => undefined); + throw error; } + await handle.close(); } -/** Creates the lock file. Returns false when another process holds it. */ -async function tryAcquire(lockFile: string): Promise { +/** Creates the lock file. Returns the record, or null when another process holds it. */ +async function tryAcquire(lockFile: string): Promise { + let handle: fs.FileHandle; try { - const handle = await fs.open(lockFile, 'wx', 0o600); - await handle.writeFile(String(process.pid)); - await handle.close(); - return true; + handle = await fs.open(lockFile, 'wx', 0o600); } catch (error) { if (errorCode(error) === 'EEXIST') { - return false; + return null; } throw error; } + const record = { nonce: randomBytes(16).toString('hex'), pid: process.pid, time: Date.now() }; + await writeRecord(handle, lockFile, record); + return record; } -async function acquire(lockFile: string): Promise { +async function acquire(lockFile: string): Promise { const deadline = Date.now() + MAX_WAIT_MS; - while (!(await tryAcquire(lockFile))) { + for (;;) { + const record = await tryAcquire(lockFile); + if (record) { + return record; + } if (Date.now() > deadline) { throw new Error(`Timed out waiting for ${lockFile}. If no other qc command is running, delete that file.`); } @@ -52,15 +150,73 @@ async function acquire(lockFile: string): Promise { } } +function ownsSync(lockFile: string, nonce: string): boolean { + try { + return parseRecord(readFileSync(lockFile, 'utf-8'))?.nonce === nonce; + } catch { + return false; + } +} + +async function owns(lockFile: string, nonce: string): Promise { + const content = await readLock(lockFile).catch(() => null); + return content !== null && parseRecord(content)?.nonce === nonce; +} + +/** Releases the lock only if this holder still owns it. */ +async function release(lockFile: string, nonce: string): Promise { + if (await owns(lockFile, nonce)) { + await fs.unlink(lockFile).catch(() => undefined); + } +} + +/** + * Removes the lock on process.exit, and defers SIGINT/SIGTERM until the locked + * work is done, so a refresh is not cut off between the request and the save. + */ +function guardProcess(lockFile: string, nonce: string): () => void { + const pending: NodeJS.Signals[] = []; + const onExit = () => { + if (ownsSync(lockFile, nonce)) { + try { + unlinkSync(lockFile); + } catch { + // Nothing more can be done while the process exits. + } + } + }; + const onSignal = (signal: NodeJS.Signals) => { + pending.push(signal); + }; + process.on('exit', onExit); + DEFERRED_SIGNALS.forEach((signal) => process.on(signal, onSignal)); + return () => { + process.off('exit', onExit); + DEFERRED_SIGNALS.forEach((signal) => process.off(signal, onSignal)); + if (pending.length > 0) { + process.kill(process.pid, pending[0]); + } + }; +} + /** * Runs fn while this process holds an exclusive lock file. Every qc process that * uses the same lock file waits for the others, in this process and in others. */ -export async function withFileLock(lockFile: string, fn: () => Promise): Promise { - await acquire(lockFile); +export async function withFileLock(lockFile: string, fn: (lock: HeldLock) => Promise): Promise { + const { nonce } = await acquire(lockFile); + const unguard = guardProcess(lockFile, nonce); + const lock: HeldLock = { + verify: async () => { + if (!(await owns(lockFile, nonce))) { + throw new Error(`Lost the lock ${lockFile} to another process`); + } + }, + }; try { - return await fn(); + return await fn(lock); } finally { - await fs.unlink(lockFile).catch(() => undefined); + await release(lockFile, nonce); + unguard(); } } From 0f1e69d52bb2d4e78e29dea95d3df18590691248 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 21:05:19 +1000 Subject: [PATCH 07/15] fix(auth): keep a rotated login when the save fails, and refresh once per process - If the save fails after the portal rotated the tokens, retry the save once, use the new pair for this command, and print where the save failed with "run qc login if later commands fail". - refreshIfDue runs at most once per platform in a process, so getEffectiveAuth and ApiClient.create never send the same refresh token twice, even after a failed save. - Check lock ownership just before the refresh request. - Refresh timeout 60 s (the portal can stall and still commits the rotation); "Renewing Quant login..." on stderr after 5 s. - The reload under the lock reads strictly: a broken file stops the refresh instead of looking like no login. - An ApiClient stops renewing after a renewed token also gets 401, so a polling loop cannot rotate the token on every request. - qc org list goes through ApiClient.getUserInfo(), so it gets the 401 retry. --- __tests__/utils/context.test.ts | 2 + __tests__/utils/oauth-refresh-flow.test.ts | 36 ++++++++++++++- src/commands/org.ts | 23 +++------- src/utils/api.ts | 15 ++++-- src/utils/auth-retry.ts | 15 ++++-- src/utils/context.ts | 36 ++++++++++++--- src/utils/token-refresh.ts | 53 +++++++++++++++++++--- 7 files changed, 143 insertions(+), 37 deletions(-) diff --git a/__tests__/utils/context.test.ts b/__tests__/utils/context.test.ts index 68cbf9f..248a436 100644 --- a/__tests__/utils/context.test.ts +++ b/__tests__/utils/context.test.ts @@ -11,6 +11,8 @@ jest.unstable_mockModule('../../src/utils/config.js', () => ({ savePlatformUpdates: mockSaveNamed, applyPlatformUpdates: mockSaveNamed, CREDENTIALS_LOCK_FILE: '/nonexistent/credentials.lock', + CONFIG_FILE: '/nonexistent/credentials', + loadCredentialsLocked: mockLoadMulti, getActivePlatformConfig: async () => { const m = await mockLoadMulti(); const { platformInfo, ...auth } = m.platforms[m.activePlatform]; diff --git a/__tests__/utils/oauth-refresh-flow.test.ts b/__tests__/utils/oauth-refresh-flow.test.ts index b22fad8..3be7412 100644 --- a/__tests__/utils/oauth-refresh-flow.test.ts +++ b/__tests__/utils/oauth-refresh-flow.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, jest, beforeAll, beforeEach, afterEach, afterAll } from '@jest/globals'; -import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'fs'; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync, promises as fsp } from 'fs'; import { tmpdir } from 'os'; import { join } from 'path'; import { createRequire } from 'module'; @@ -194,6 +194,19 @@ describe('401 retry', () => { expect(tokenCalls()).toHaveLength(1); }); + it('stops renewing in a client after the renewed token is refused too', async () => { + writeLogin({ token: 'qc_old8', refreshToken: 'qcr_old8', expiresAt: notDue() }); + fetchMock.mockResolvedValue(rotated(8)); + const sent = fakeApi(() => 401); + + const client = await ApiClient.create(); + await expect(client.getApplications()).rejects.toThrow(/Authentication expired/); + await expect(client.getApplications()).rejects.toThrow(/Authentication expired/); + + expect(sent).toEqual(['qc_old8', 'qc_new8', 'qc_new8']); + expect(tokenCalls()).toHaveLength(1); + }); + it('refreshes once and repeats the /api/oauth/user request once', async () => { writeLogin({ token: 'qc_old6', refreshToken: 'qcr_old6', expiresAt: notDue() }); fetchMock.mockImplementation(async (url: any, init: any) => { @@ -211,6 +224,27 @@ describe('401 retry', () => { }); }); +describe('save failure after a rotation', () => { + it('uses the new pair, refreshes once in this process, and reports the path', async () => { + writeLogin({ token: 'qc_old9', refreshToken: 'qcr_old9', expiresAt: new Date(Date.now() + DAY).toISOString() }); + fetchMock.mockResolvedValue(rotated(9)); + const write = jest.spyOn(fsp, 'writeFile').mockRejectedValue(Object.assign(new Error('disk full'), { code: 'ENOSPC' })); + const error = jest.spyOn(console, 'error').mockImplementation(() => {}); + + const first = await ctx.getEffectiveAuth(); + const second = await ctx.getEffectiveAuth(); + + expect(first?.token).toBe('qc_new9'); + expect(second?.token).toBe('qc_new9'); + expect(tokenCalls()).toHaveLength(1); + expect(write).toHaveBeenCalledTimes(2); + expect(error).toHaveBeenCalledTimes(1); + expect(String(error.mock.calls[0][0])).toContain(`Could not save the refreshed login to ${credFile}`); + expect(String(error.mock.calls[0][0])).toContain('qc login'); + expect(stored().token).toBe('qc_old9'); + }); +}); + describe('refresh token refused', () => { it('tells the user to run qc login and keeps the credentials', async () => { writeLogin({ token: 'qc_old7', refreshToken: 'qcr_dead', expiresAt: new Date(Date.now() + DAY).toISOString() }); diff --git a/src/commands/org.ts b/src/commands/org.ts index 5bd1ff0..54034bd 100644 --- a/src/commands/org.ts +++ b/src/commands/org.ts @@ -3,6 +3,7 @@ import chalk from 'chalk'; import inquirer from 'inquirer'; import { createSpinner } from '../utils/spinner.js'; import { getEffectiveAuth, saveEffectiveConfig } from '../utils/context.js'; +import { ApiClient } from '../utils/api.js'; import { UserInfo } from '../types/auth.js'; import { Logger } from '../utils/logger.js'; @@ -45,24 +46,14 @@ async function handleOrgList() { let userInfo: UserInfo; try { - const response = await fetch(`${auth.host}/api/oauth/user`, { - headers: { - 'Authorization': `Bearer ${auth.token}`, - 'Accept': 'application/json' - } - }); - - if (!response.ok) { - spinner.fail('Failed to fetch organization data'); - logger.error('Failed to fetch organization data. Please re-authenticate with `quant-cloud login`.'); - return; - } - - userInfo = await response.json() as UserInfo; + // ApiClient renews the login once after a 401. + const client = await ApiClient.create(); + userInfo = await client.getUserInfo() as UserInfo; spinner.succeed('Loaded organizations'); } catch (error) { - spinner.fail('Failed to load organizations'); - throw error; + spinner.fail('Failed to fetch organization data'); + logger.error(error instanceof Error ? error.message : String(error)); + return; } if (!userInfo.organizations || userInfo.organizations.length === 0) { diff --git a/src/utils/api.ts b/src/utils/api.ts index c8324c5..fdc312d 100644 --- a/src/utils/api.ts +++ b/src/utils/api.ts @@ -40,6 +40,8 @@ export class ApiClient { private defaultEnvironmentId?: string; private token: string; private renewToken?: TokenRenewer; + /** Set when a renewed token was refused too. Then this client stops renewing. */ + private renewalRejected = false; constructor(baseUrl: string, token: string, defaultOrganizationId?: string, defaultApplicationId?: string, defaultEnvironmentId?: string, renewToken?: TokenRenewer) { this.token = token; @@ -51,7 +53,9 @@ export class ApiClient { accessToken: () => this.token }); // Legacy logins (no refresh token) keep the default axios and never retry. - const http = renewToken ? createRenewingAxios((sent) => this.renew(sent)) : undefined; + const http = renewToken + ? createRenewingAxios((sent) => this.renew(sent), () => { this.renewalRejected = true; }) + : undefined; this.applicationsApi = new ApplicationsApi(config, undefined, http); this.environmentsApi = new EnvironmentsApi(config, undefined, http); @@ -84,7 +88,7 @@ export class ApiClient { /** Renews the token after a 401 for sentToken. Returns the new token, or null. */ private async renew(sentToken: string): Promise { - if (!this.renewToken) { + if (!this.renewToken || this.renewalRejected) { return null; } const token = await this.renewToken(sentToken); @@ -177,7 +181,12 @@ export class ApiClient { return response; } const token = await this.renew(sent); - return token ? this.fetchUserInfo(token) : response; + if (!token) { + return response; + } + const retried = await this.fetchUserInfo(token); + this.renewalRejected = retried.status === 401; + return retried; } async getUserInfo(): Promise { diff --git a/src/utils/auth-retry.ts b/src/utils/auth-retry.ts index 664f682..0f417bb 100644 --- a/src/utils/auth-retry.ts +++ b/src/utils/auth-retry.ts @@ -9,9 +9,13 @@ function sentToken(config: RetryConfig): string { return String(config.headers?.Authorization ?? '').replace(/^Bearer\s+/i, ''); } -async function retryAfterRenewal(http: AxiosInstance, renew: Renew, error: AxiosError): Promise { +async function retryAfterRenewal(http: AxiosInstance, renew: Renew, onRejected: () => void, error: AxiosError): Promise { const config = error?.config as RetryConfig | undefined; - if (error?.response?.status !== 401 || !config || config._qcRenewed) { + if (error?.response?.status !== 401 || !config) { + throw error; + } + if (config._qcRenewed) { + onRejected(); throw error; } config._qcRenewed = true; @@ -25,10 +29,11 @@ async function retryAfterRenewal(http: AxiosInstance, renew: Renew, error: Axios /** * An axios instance for the quant-client APIs. After a 401 it renews the token - * once and sends the same request again. A second 401 goes to the caller. + * once and sends the same request again. A second 401 goes to the caller, and + * onRejected is called: the renewed token was refused too. */ -export function createRenewingAxios(renew: Renew): AxiosInstance { +export function createRenewingAxios(renew: Renew, onRejected: () => void): AxiosInstance { const http = axios.create(); - http.interceptors.response.use(undefined, (error) => retryAfterRenewal(http, renew, error)); + http.interceptors.response.use(undefined, (error) => retryAfterRenewal(http, renew, onRejected, error)); return http; } diff --git a/src/utils/context.ts b/src/utils/context.ts index 8ce6095..3c2e8ba 100644 --- a/src/utils/context.ts +++ b/src/utils/context.ts @@ -1,8 +1,8 @@ -import { applyPlatformUpdates, CREDENTIALS_LOCK_FILE, loadMultiPlatformConfig, saveActivePlatformConfig, savePlatformUpdates } from './config.js'; +import { applyPlatformUpdates, CONFIG_FILE, CREDENTIALS_LOCK_FILE, loadCredentialsLocked, loadMultiPlatformConfig, saveActivePlatformConfig, savePlatformUpdates } from './config.js'; import type { AuthConfig } from '../types/auth.js'; import { getProjectConfig, ProjectConfig } from './project-config.js'; import { Logger } from './logger.js'; -import { forceRefresh, refreshIfDue, RefreshStore } from './token-refresh.js'; +import { forceRefresh, isRefreshDue, refreshIfDue, RefreshStore } from './token-refresh.js'; const logger = new Logger('Context'); @@ -33,8 +33,8 @@ export interface EffectiveContext { export type TokenRenewer = (sentToken: string) => Promise; /** The stored login for a platform, or null when there is none. */ -async function loadStoredAuth(platformId: string): Promise { - const multiConfig = await loadMultiPlatformConfig(); +async function loadStoredAuth(platformId: string, load = loadMultiPlatformConfig): Promise { + const multiConfig = await load(); const stored = multiConfig.platforms[platformId]; if (!stored) { return null; @@ -60,12 +60,36 @@ async function loadPlatformAuth(platformId: string): Promise { */ export function platformRefreshStore(platformId: string): RefreshStore { return { - reload: () => loadStoredAuth(platformId), + // Called under the lock: a file that cannot be read throws instead of looking empty. + reload: () => loadStoredAuth(platformId, loadCredentialsLocked), save: (updates) => applyPlatformUpdates(platformId, updates), lockFile: CREDENTIALS_LOCK_FILE, + location: CONFIG_FILE, }; } +/** The proactive refresh of each platform in this process, keyed by platform id. */ +const refreshes = new Map }>(); + +/** + * refreshIfDue, at most once for each platform in this process. A second call + * for the same stored token (for example when the save failed) gets the first + * result, so this process never sends the same refresh token twice. + */ +async function refreshOncePerProcess(platformId: string, auth: AuthConfig): Promise { + const earlier = refreshes.get(platformId); + if (earlier && earlier.from === auth.token) { + const { token, refreshToken, expiresAt } = await earlier.result; + return { ...auth, token, refreshToken, expiresAt }; + } + if (!isRefreshDue(auth)) { + return auth; + } + const result = refreshIfDue(auth, platformRefreshStore(platformId)); + refreshes.set(platformId, { from: auth.token, result }); + return result; +} + /** A TokenRenewer for this login, or undefined for a legacy login without a refresh token. */ export function tokenRenewer(auth: AuthConfig, platformId: string): TokenRenewer | undefined { if (!auth.refreshToken) { @@ -125,7 +149,7 @@ async function loadEffectiveAuth(platformFlag?: string): Promise<{ platformId: s if (!auth) { return null; } - return { platformId, auth: await refreshIfDue(auth, platformRefreshStore(platformId)) }; + return { platformId, auth: await refreshOncePerProcess(platformId, auth) }; } /** diff --git a/src/utils/token-refresh.ts b/src/utils/token-refresh.ts index cd0aef9..41d5bdb 100644 --- a/src/utils/token-refresh.ts +++ b/src/utils/token-refresh.ts @@ -1,5 +1,5 @@ import type { AuthConfig, LoginResponse } from '../types/auth.js'; -import { withFileLock } from './file-lock.js'; +import { HeldLock, withFileLock } from './file-lock.js'; import { Logger } from './logger.js'; const logger = new Logger('Auth'); @@ -10,10 +10,15 @@ const CLIENT_ID = 'quant-cli'; export const REFRESH_WINDOW_MS = 7 * 24 * 60 * 60 * 1000; /** - * The refresh request must end well before LOCK_STALE_MS, or a waiting process - * takes the lock and sends the same refresh token, which revokes the chain. + * The portal can stall for a long time, and it commits a rotation even when the + * client has gone, so qc waits long for the answer. The timeout must stay below + * LOCK_STALE_MS, or a waiting process takes the lock and sends the same refresh + * token, which revokes the chain. */ -const REFRESH_TIMEOUT_MS = 15_000; +export const REFRESH_TIMEOUT_MS = 60_000; + +/** After this long, qc tells the user that it is still renewing the login. */ +const SLOW_NOTICE_MS = 5_000; /** Refresh tokens the portal refused in this process. They are not sent again. */ const rejectedTokens = new Set(); @@ -26,6 +31,8 @@ export interface RefreshStore { reload(): Promise; /** The lock file that every qc process holds while it refreshes or saves. */ lockFile: string; + /** The credentials file, for messages. */ + location: string; } /** The portal refused the refresh token (HTTP 400 invalid_grant or invalid_client). */ @@ -77,7 +84,23 @@ function refreshFailure(status: number, body: TokenErrorBody): Error { return new Error(`HTTP ${status}`); } +/** Prints a note on stderr when the refresh takes longer than SLOW_NOTICE_MS. Returns a function that cancels it. */ +function noticeIfSlow(): () => void { + const timer = setTimeout(() => process.stderr.write('Renewing Quant login…\n'), SLOW_NOTICE_MS); + timer.unref?.(); + return () => clearTimeout(timer); +} + async function requestRefresh(auth: AuthConfig): Promise> { + const cancelNotice = noticeIfSlow(); + try { + return await sendRefresh(auth); + } finally { + cancelNotice(); + } +} + +async function sendRefresh(auth: AuthConfig): Promise> { const refreshToken = auth.refreshToken as string; if (rejectedTokens.has(refreshToken)) { throw new TokenRefreshError('invalid_grant', 'Refresh token was refused earlier'); @@ -117,18 +140,36 @@ async function refreshLocked( store: RefreshStore, shouldRefresh: (latest: AuthConfig) => boolean, ): Promise { - return withFileLock(store.lockFile, async () => { + return withFileLock(store.lockFile, async (lock: HeldLock) => { const latest = await store.reload(); if (!latest || latest.token !== auth.token || !shouldRefresh(latest)) { return latest ?? auth; } + await lock.verify(); const updates = await requestRefresh(latest); - await store.save(updates); + await saveRotation(store, updates); logger.debug('Access token refreshed'); return { ...latest, ...updates }; }); } +/** + * Saves a rotated pair, with one retry. The portal has already rotated, so the + * new pair is used for this command even when it cannot be saved. + */ +async function saveRotation(store: RefreshStore, updates: Partial): Promise { + try { + await store.save(updates); + } catch { + try { + await store.save(updates); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + logger.error(`Could not save the refreshed login to ${store.location} (${reason}): run \`qc login\` if later commands fail.`); + } + } +} + let warnedRejected = false; function reportFailure(auth: AuthConfig, error: unknown): void { From 62376c650f7a4ea2ede8714ea36c3cb15d98f294 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 21:05:19 +1000 Subject: [PATCH 08/15] fix(whoami): show expired when the access token expiry is in the past --- __tests__/utils/whoami-expiry.test.ts | 5 +++++ src/commands/whoami.ts | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/__tests__/utils/whoami-expiry.test.ts b/__tests__/utils/whoami-expiry.test.ts index ce31e7a..af8da61 100644 --- a/__tests__/utils/whoami-expiry.test.ts +++ b/__tests__/utils/whoami-expiry.test.ts @@ -9,6 +9,11 @@ describe('formatTimeLeft', () => { expect(formatTimeLeft(49 * HOUR)).toBe('2 days'); }); + it('shows expired when no time is left', () => { + expect(formatTimeLeft(0)).toBe('expired'); + expect(formatTimeLeft(-HOUR)).toBe('expired'); + }); + it('shows hours and minutes for 48 hours or less', () => { expect(formatTimeLeft(48 * HOUR)).toBe('48h 0m'); expect(formatTimeLeft(5 * HOUR + 20 * 60 * 1000)).toBe('5h 20m'); diff --git a/src/commands/whoami.ts b/src/commands/whoami.ts index 1e59354..752c9d9 100644 --- a/src/commands/whoami.ts +++ b/src/commands/whoami.ts @@ -17,8 +17,11 @@ export function whoamiCommand(program: Command) { const HOUR_MS = 60 * 60 * 1000; const DAY_MS = 24 * HOUR_MS; -/** "12 days" when more than 48 hours are left, else "5h 20m". */ +/** "12 days" when more than 48 hours are left, "5h 20m" when less, "expired" when none. */ export function formatTimeLeft(ms: number): string { + if (ms <= 0) { + return 'expired'; + } if (ms > 48 * HOUR_MS) { return `${Math.floor(ms / DAY_MS)} days`; } @@ -32,9 +35,11 @@ function printExpiry(expiresAt?: string): void { return; } const timeLeft = new Date(expiresAt).getTime() - Date.now(); - if (timeLeft > 0) { - logger.info(`Expires in: ${chalk.green(formatTimeLeft(timeLeft))}`); + if (timeLeft <= 0) { + logger.info(`Expires in: ${chalk.red(formatTimeLeft(timeLeft))}`); + return; } + logger.info(`Expires in: ${chalk.green(formatTimeLeft(timeLeft))}`); } function printActiveOrganization(ctx: EffectiveContext, organizations: NonNullable): void { From 008f5a602d66c39d026fc21edf90187eb3c40543 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 21:05:19 +1000 Subject: [PATCH 09/15] chore: require Node 18 (global fetch and AbortSignal.timeout) --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5dac4ac..059c732 100644 --- a/package-lock.json +++ b/package-lock.json @@ -52,7 +52,7 @@ "typescript": "^5.2.2" }, "engines": { - "node": ">=16.0.0" + "node": ">=18.0.0" } }, "node_modules/@babel/code-frame": { diff --git a/package.json b/package.json index 2777b82..4cb8231 100644 --- a/package.json +++ b/package.json @@ -95,7 +95,7 @@ "picomatch": "^4.0.4" }, "engines": { - "node": ">=16.0.0" + "node": ">=18.0.0" }, "files": [ "dist/**/*", From 584d798ac8e2cd76d1819560c133f4510dca8410 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 21:07:34 +1000 Subject: [PATCH 10/15] test(auth): the lock ordering test accepts either holder first --- __tests__/utils/file-lock.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/__tests__/utils/file-lock.test.ts b/__tests__/utils/file-lock.test.ts index e34ff36..17f338e 100644 --- a/__tests__/utils/file-lock.test.ts +++ b/__tests__/utils/file-lock.test.ts @@ -30,7 +30,10 @@ describe('withFileLock', () => { events.push(`${name}-end`); }); await Promise.all([work('a'), work('b')]); - expect(events).toEqual(['a-start', 'a-end', 'b-start', 'b-end']); + // Either holder can go first, but they must not overlap. + const first = events[0].split('-')[0]; + const second = first === 'a' ? 'b' : 'a'; + expect(events).toEqual([`${first}-start`, `${first}-end`, `${second}-start`, `${second}-end`]); }); it('takes over a lock whose holder timestamp is stale', async () => { From 32c169673084ac10a1edbcb29f4e5223c93aa59d Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Fri, 6 Nov 2026 06:50:35 +1000 Subject: [PATCH 11/15] feat(auth): retry a refresh once after an unknown outcome The portal now recovers a lost rotation: the previous refresh token, sent again within 5 minutes and before the new access token was used, gets a new pair once. After an unknown outcome (timeout or abort, network error, 5xx, or a 200 body that cannot be read), qc keeps the lock, waits 20 s and sends the same refresh token once more. A 200 is saved as usual. Any other answer keeps the earlier behaviour. There is never more than one retry, and no retry after a definite 4xx. Both attempts and the wait can take up to 140 s, longer than the 90 s stale limit, so HeldLock.touch() verifies ownership and writes a new holder timestamp before and after the wait. --- __tests__/utils/file-lock.test.ts | 14 ++++ __tests__/utils/oauth-refresh-flow.test.ts | 1 + __tests__/utils/token-refresh.test.ts | 89 ++++++++++++++++++++-- src/utils/file-lock.ts | 18 ++++- src/utils/token-refresh.ts | 78 ++++++++++++++++--- 5 files changed, 176 insertions(+), 24 deletions(-) diff --git a/__tests__/utils/file-lock.test.ts b/__tests__/utils/file-lock.test.ts index 17f338e..d81d7b7 100644 --- a/__tests__/utils/file-lock.test.ts +++ b/__tests__/utils/file-lock.test.ts @@ -87,4 +87,18 @@ describe('withFileLock', () => { expect(during).toBe(before + 1); expect(process.listenerCount('exit')).toBe(before); }); + + it('touch() writes a new timestamp, and fails when the lock was taken over', async () => { + const lockFile = nextLock(); + await withFileLock(lockFile, async (lock) => { + const before = JSON.parse(readFileSync(lockFile, 'utf-8')); + await new Promise((resolve) => setTimeout(resolve, 20)); + await lock.touch(); + const after = JSON.parse(readFileSync(lockFile, 'utf-8')); + expect(after.nonce).toBe(before.nonce); + expect(after.time).toBeGreaterThan(before.time); + writeFileSync(lockFile, record(Date.now(), 'someone-else')); + await expect(lock.touch()).rejects.toThrow(/Lost the lock/); + }); + }); }); diff --git a/__tests__/utils/oauth-refresh-flow.test.ts b/__tests__/utils/oauth-refresh-flow.test.ts index 3be7412..83c99b6 100644 --- a/__tests__/utils/oauth-refresh-flow.test.ts +++ b/__tests__/utils/oauth-refresh-flow.test.ts @@ -28,6 +28,7 @@ beforeAll(async () => { ({ CONFIG_FILE: credFile } = await import('../../src/utils/config.js')); ctx = await import('../../src/utils/context.js'); ({ ApiClient } = await import('../../src/utils/api.js')); + (await import('../../src/utils/token-refresh.js')).recovery.delayMs = 5; }); afterAll(() => rmSync(home, { recursive: true, force: true })); diff --git a/__tests__/utils/token-refresh.test.ts b/__tests__/utils/token-refresh.test.ts index 2bcdc8f..0155797 100644 --- a/__tests__/utils/token-refresh.test.ts +++ b/__tests__/utils/token-refresh.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, jest, beforeEach, afterEach, afterAll } from '@je import { mkdtempSync, rmSync } from 'fs'; import { tmpdir } from 'os'; import { join } from 'path'; -import { forceRefresh, isRefreshDue, refreshIfDue, toAuthUpdates, REFRESH_WINDOW_MS } from '../../src/utils/token-refresh.js'; +import { forceRefresh, isRefreshDue, recovery, refreshIfDue, toAuthUpdates, REFRESH_WINDOW_MS } from '../../src/utils/token-refresh.js'; import type { AuthConfig } from '../../src/types/auth.js'; const DAY = 24 * 60 * 60 * 1000; @@ -65,22 +65,24 @@ describe('refreshIfDue', () => { global.fetch = fetchMock as any; save.mockResolvedValue(undefined); reload.mockImplementation(async () => login(DAY)); + recovery.delayMs = 5; }); afterEach(() => { global.fetch = realFetch; + jest.restoreAllMocks(); }); it('does not call the portal when no refresh is due', async () => { const auth = login(30 * DAY); - expect(await refreshIfDue(auth, { save, reload, lockFile })).toBe(auth); + expect(await refreshIfDue(auth, { save, reload, lockFile, location: 'creds' })).toBe(auth); expect(fetchMock).not.toHaveBeenCalled(); }); it('never refreshes a legacy login, even after a 401', async () => { const legacy: AuthConfig = { token: 'legacy', host: HOST }; - expect(await refreshIfDue(legacy, { save, reload, lockFile })).toBe(legacy); - expect(await forceRefresh(legacy, { save, reload, lockFile })).toBeNull(); + expect(await refreshIfDue(legacy, { save, reload, lockFile, location: 'creds' })).toBe(legacy); + expect(await forceRefresh(legacy, { save, reload, lockFile, location: 'creds' })).toBeNull(); expect(fetchMock).not.toHaveBeenCalled(); expect(reload).not.toHaveBeenCalled(); }); @@ -90,7 +92,7 @@ describe('refreshIfDue', () => { access_token: 'qc_new', refresh_token: 'qcr_new', expires_in: 90 * 86400, token_type: 'Bearer', })); - const result = await refreshIfDue(login(DAY), { save, reload, lockFile }); + const result = await refreshIfDue(login(DAY), { save, reload, lockFile, location: 'creds' }); expect(result.token).toBe('qc_new'); expect(result.refreshToken).toBe('qcr_new'); @@ -106,7 +108,7 @@ describe('refreshIfDue', () => { it('uses the credentials another process saved, and sends no request', async () => { reload.mockResolvedValue(login(90 * DAY, { token: 'qc_peer', refreshToken: 'qcr_peer' })); - const result = await refreshIfDue(login(DAY), { save, reload, lockFile }); + const result = await refreshIfDue(login(DAY), { save, reload, lockFile, location: 'creds' }); expect(result.token).toBe('qc_peer'); expect(fetchMock).not.toHaveBeenCalled(); @@ -117,7 +119,7 @@ describe('refreshIfDue', () => { reload.mockResolvedValue(login(60 * DAY)); fetchMock.mockResolvedValue(jsonResponse(200, { access_token: 'qc_new', refresh_token: 'qcr_new', expires_in: 60, token_type: 'Bearer' })); - const result = await forceRefresh(login(60 * DAY), { save, reload, lockFile }); + const result = await forceRefresh(login(60 * DAY), { save, reload, lockFile, location: 'creds' }); expect(result?.token).toBe('qc_new'); expect(fetchMock).toHaveBeenCalledTimes(1); @@ -128,7 +130,78 @@ describe('refreshIfDue', () => { const auth = login(DAY); reload.mockResolvedValue(auth); - expect(await refreshIfDue(auth, { save, reload, lockFile })).toBe(auth); + expect(await refreshIfDue(auth, { save, reload, lockFile, location: 'creds' })).toBe(auth); expect(save).not.toHaveBeenCalled(); }); + + describe('unknown outcome recovery', () => { + const ok = (n: string) => jsonResponse(200, { access_token: `qc_${n}`, refresh_token: `qcr_${n}`, expires_in: 60, token_type: 'Bearer' }); + const sentTokens = () => fetchMock.mock.calls.map((c: any[]) => new URLSearchParams(c[1].body).get('refresh_token')); + + it('retries once with the same refresh token after a timeout, and saves the recovered pair', async () => { + fetchMock + .mockRejectedValueOnce(Object.assign(new Error('The operation was aborted due to timeout'), { name: 'TimeoutError' })) + .mockResolvedValueOnce(ok('recovered')); + + const result = await refreshIfDue(login(DAY, { refreshToken: 'qcr_t1' }), { save, reload: async () => login(DAY, { refreshToken: 'qcr_t1' }), lockFile, location: 'creds' }); + + expect(result.token).toBe('qc_recovered'); + expect(sentTokens()).toEqual(['qcr_t1', 'qcr_t1']); + expect(save).toHaveBeenCalledWith(expect.objectContaining({ token: 'qc_recovered', refreshToken: 'qcr_recovered' })); + }); + + it('retries once after a 5xx', async () => { + fetchMock.mockResolvedValueOnce(jsonResponse(503, {})).mockResolvedValueOnce(ok('after5xx')); + + const result = await refreshIfDue(login(DAY), { save, reload, lockFile, location: 'creds' }); + + expect(result.token).toBe('qc_after5xx'); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('retries once after an unreadable 200, and keeps the login when the retry is refused', async () => { + fetchMock + .mockResolvedValueOnce({ ok: true, status: 200, json: async () => { throw new Error('bad json'); } }) + .mockResolvedValueOnce(jsonResponse(400, { error: 'invalid_grant', error_description: 'not found' })); + jest.spyOn(console, 'warn').mockImplementation(() => {}); + const auth = login(DAY, { refreshToken: 'qcr_unreadable' }); + + const result = await refreshIfDue(auth, { save, reload: async () => auth, lockFile, location: 'creds' }); + + expect(result).toBe(auth); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(save).not.toHaveBeenCalled(); + }); + + it('never retries more than once', async () => { + fetchMock.mockRejectedValue(new Error('socket hang up')); + const auth = login(DAY, { refreshToken: 'qcr_twice' }); + + expect(await refreshIfDue(auth, { save, reload: async () => auth, lockFile, location: 'creds' })).toBe(auth); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('does not retry after a definite 4xx', async () => { + fetchMock.mockResolvedValue(jsonResponse(400, { error: 'invalid_grant' })); + jest.spyOn(console, 'warn').mockImplementation(() => {}); + const auth = login(DAY, { refreshToken: 'qcr_definite' }); + + await refreshIfDue(auth, { save, reload: async () => auth, lockFile, location: 'creds' }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('waits the recovery delay before the retry', async () => { + recovery.delayMs = 200; + const times: number[] = []; + fetchMock.mockImplementation(async () => { + times.push(Date.now()); + return times.length === 1 ? jsonResponse(502, {}) : ok('late'); + }); + + await refreshIfDue(login(DAY), { save, reload, lockFile, location: 'creds' }); + + expect(times[1] - times[0]).toBeGreaterThanOrEqual(190); + }); + }); }); diff --git a/src/utils/file-lock.ts b/src/utils/file-lock.ts index ee385f7..41cfff0 100644 --- a/src/utils/file-lock.ts +++ b/src/utils/file-lock.ts @@ -22,6 +22,11 @@ interface LockRecord { export interface HeldLock { /** Throws when another process has taken the lock over. */ verify(): Promise; + /** + * Verifies ownership and writes a new timestamp, so a holder that must keep + * the lock longer than LOCK_STALE_MS is not taken over. + */ + touch(): Promise; } const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); @@ -206,11 +211,16 @@ function guardProcess(lockFile: string, nonce: string): () => void { export async function withFileLock(lockFile: string, fn: (lock: HeldLock) => Promise): Promise { const { nonce } = await acquire(lockFile); const unguard = guardProcess(lockFile, nonce); + const verify = async () => { + if (!(await owns(lockFile, nonce))) { + throw new Error(`Lost the lock ${lockFile} to another process`); + } + }; const lock: HeldLock = { - verify: async () => { - if (!(await owns(lockFile, nonce))) { - throw new Error(`Lost the lock ${lockFile} to another process`); - } + verify, + touch: async () => { + await verify(); + await fs.writeFile(lockFile, JSON.stringify({ nonce, pid: process.pid, time: Date.now() })); }, }; try { diff --git a/src/utils/token-refresh.ts b/src/utils/token-refresh.ts index 41d5bdb..d0e1d69 100644 --- a/src/utils/token-refresh.ts +++ b/src/utils/token-refresh.ts @@ -17,6 +17,13 @@ export const REFRESH_WINDOW_MS = 7 * 24 * 60 * 60 * 1000; */ export const REFRESH_TIMEOUT_MS = 60_000; +/** + * After an unknown outcome, qc waits this long and sends the same refresh token + * once more. The portal recovers a rotation whose answer was lost. Tests may + * lower it. + */ +export const recovery = { delayMs: 20_000 }; + /** After this long, qc tells the user that it is still renewing the login. */ const SLOW_NOTICE_MS = 5_000; @@ -35,6 +42,17 @@ export interface RefreshStore { location: string; } +/** + * The portal may have rotated the tokens, but qc did not get the new pair: a + * timeout, a network error, a 5xx, or a 200 body that cannot be read. + */ +export class UnknownRefreshOutcome extends Error { + constructor(reason: string) { + super(reason); + this.name = 'UnknownRefreshOutcome'; + } +} + /** The portal refused the refresh token (HTTP 400 invalid_grant or invalid_client). */ export class TokenRefreshError extends Error { constructor(public readonly code: string, description?: string) { @@ -78,6 +96,9 @@ interface TokenErrorBody { } function refreshFailure(status: number, body: TokenErrorBody): Error { + if (status >= 500 || (status >= 200 && status < 300)) { + return new UnknownRefreshOutcome(`HTTP ${status} without a usable token response`); + } if (body.error) { return new TokenRefreshError(body.error, body.error_description); } @@ -105,7 +126,29 @@ async function sendRefresh(auth: AuthConfig): Promise> { if (rejectedTokens.has(refreshToken)) { throw new TokenRefreshError('invalid_grant', 'Refresh token was refused earlier'); } - const response = await fetch(new URL('/oauth/token', auth.host).toString(), { + const response = await postRefresh(auth.host, refreshToken); + const body = await response.json().catch(() => ({})) as Partial & TokenErrorBody; + if (!response.ok || typeof body.access_token !== 'string') { + const failure = refreshFailure(response.status, body); + if (failure instanceof TokenRefreshError) { + rejectedTokens.add(refreshToken); + } + throw failure; + } + return toAuthUpdates(body as LoginResponse); +} + +/** Sends the refresh request. A timeout or network error is an unknown outcome. */ +async function postRefresh(host: string, refreshToken: string): Promise { + try { + return await sendRefreshRequest(host, refreshToken); + } catch (error) { + throw new UnknownRefreshOutcome(error instanceof Error ? error.message : String(error)); + } +} + +function sendRefreshRequest(host: string, refreshToken: string): Promise { + return fetch(new URL('/oauth/token', host).toString(), { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', @@ -118,16 +161,6 @@ async function sendRefresh(auth: AuthConfig): Promise> { }).toString(), signal: AbortSignal.timeout(REFRESH_TIMEOUT_MS), }); - - const body = await response.json().catch(() => ({})) as Partial & TokenErrorBody; - if (!response.ok || typeof body.access_token !== 'string') { - const failure = refreshFailure(response.status, body); - if (failure instanceof TokenRefreshError) { - rejectedTokens.add(refreshToken); - } - throw failure; - } - return toAuthUpdates(body as LoginResponse); } /** @@ -146,13 +179,34 @@ async function refreshLocked( return latest ?? auth; } await lock.verify(); - const updates = await requestRefresh(latest); + const updates = await refreshWithRecovery(latest, lock); await saveRotation(store, updates); logger.debug('Access token refreshed'); return { ...latest, ...updates }; }); } +/** + * requestRefresh, sent once more with the same refresh token after an unknown + * outcome. The portal answers a lost rotation with a new pair. Never more than + * one retry, and never after a definite 4xx answer. The lock is kept fresh, + * because both attempts and the wait take longer than LOCK_STALE_MS. + */ +async function refreshWithRecovery(auth: AuthConfig, lock: HeldLock): Promise> { + try { + return await requestRefresh(auth); + } catch (error) { + if (!(error instanceof UnknownRefreshOutcome)) { + throw error; + } + logger.debug(`Refresh outcome unknown (${error.message}); retrying once in ${recovery.delayMs} ms`); + await lock.touch(); + await new Promise((resolve) => setTimeout(resolve, recovery.delayMs)); + await lock.touch(); + return requestRefresh(auth); + } +} + /** * Saves a rotated pair, with one retry. The portal has already rotated, so the * new pair is used for this command even when it cannot be saved. From d2d125ac88fe49ea2b525334288ce75b089cef25 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Fri, 6 Nov 2026 07:18:41 +1000 Subject: [PATCH 12/15] fix(auth): match the portal's spaced recovery rule for lost rotations The portal answers the previous refresh token with "just rotated" for 30 s after a rotation, recovers a lost rotation once from 30 s to 5 minutes, and revokes after that. - invalid_grant "just rotated" is an unknown outcome, not a refusal: the token is not marked as refused and no "run qc login" warning is shown. - After an unknown outcome, qc waits 45 s after the previous attempt ended and sends the same refresh token again: at most 2 retries, and none when 4 minutes have passed since the first send. - Before each retry, qc reloads the credentials under the lock. If the stored token changed, it uses the stored tokens and sends nothing. - The lock is touched before and after each wait. The longest gap is one 60 s attempt, inside the 90 s stale limit. A waiter now waits up to 6 minutes, longer than the longest hold (about 5 minutes). - While the lock is held, a second Ctrl-C stops at once and removes the lock first (a signal kill emits no 'exit'). --- __tests__/utils/token-refresh.test.ts | 84 +++++++++++++++++++++++--- src/utils/file-lock.ts | 27 +++++++-- src/utils/token-refresh.ts | 87 +++++++++++++++++++++------ 3 files changed, 163 insertions(+), 35 deletions(-) diff --git a/__tests__/utils/token-refresh.test.ts b/__tests__/utils/token-refresh.test.ts index 0155797..3f93f09 100644 --- a/__tests__/utils/token-refresh.test.ts +++ b/__tests__/utils/token-refresh.test.ts @@ -65,7 +65,7 @@ describe('refreshIfDue', () => { global.fetch = fetchMock as any; save.mockResolvedValue(undefined); reload.mockImplementation(async () => login(DAY)); - recovery.delayMs = 5; + Object.assign(recovery, { delayMs: 5, maxRetries: 2, windowMs: 240_000 }); }); afterEach(() => { @@ -173,14 +173,72 @@ describe('refreshIfDue', () => { expect(save).not.toHaveBeenCalled(); }); - it('never retries more than once', async () => { + it('sends at most two retries', async () => { fetchMock.mockRejectedValue(new Error('socket hang up')); - const auth = login(DAY, { refreshToken: 'qcr_twice' }); + const auth = login(DAY, { refreshToken: 'qcr_three' }); expect(await refreshIfDue(auth, { save, reload: async () => auth, lockFile, location: 'creds' })).toBe(auth); + expect(fetchMock).toHaveBeenCalledTimes(3); + expect(save).not.toHaveBeenCalled(); + }); + + it('sends no retry after the window since the first send has passed', async () => { + Object.assign(recovery, { delayMs: 100, windowMs: 150 }); + fetchMock.mockResolvedValue(jsonResponse(503, {})); + const auth = login(DAY, { refreshToken: 'qcr_window' }); + + await refreshIfDue(auth, { save, reload: async () => auth, lockFile, location: 'creds' }); + expect(fetchMock).toHaveBeenCalledTimes(2); }); + it('skips the retry when the stored token changed during the wait', async () => { + fetchMock.mockResolvedValue(jsonResponse(502, {})); + const auth = login(DAY, { refreshToken: 'qcr_changed' }); + const stored = [auth, login(90 * DAY, { token: 'qc_other', refreshToken: 'qcr_other' })]; + const reloadSeq = jest.fn<(...args: any[]) => any>() + .mockResolvedValueOnce(stored[0]) + .mockResolvedValue(stored[1]); + + const result = await refreshIfDue(auth, { save, reload: reloadSeq, lockFile, location: 'creds' }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result.token).toBe('qc_other'); + expect(result.refreshToken).toBe('qcr_other'); + }); + + it('treats "just rotated" as unknown: no warning, not refused, and the retry recovers', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const auth = login(DAY, { refreshToken: 'qcr_just' }); + const justRotated = () => jsonResponse(400, { + error: 'invalid_grant', + error_description: 'Refresh token was just rotated; reload your stored tokens.', + }); + fetchMock.mockResolvedValueOnce(justRotated()).mockResolvedValueOnce(ok('recovered2')); + + const result = await refreshIfDue(auth, { save, reload: async () => auth, lockFile, location: 'creds' }); + + expect(result.token).toBe('qc_recovered2'); + expect(sentTokens()).toEqual(['qcr_just', 'qcr_just']); + expect(warn).not.toHaveBeenCalled(); + }); + + it('does not mark a "just rotated" token as refused', async () => { + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const auth = login(DAY, { refreshToken: 'qcr_just2' }); + fetchMock.mockResolvedValue(jsonResponse(400, { error: 'invalid_grant', error_description: 'Refresh token was just rotated' })); + const store = { save, reload: async () => auth, lockFile, location: 'creds' }; + + await refreshIfDue(auth, store); + fetchMock.mockClear(); + fetchMock.mockResolvedValue(ok('later')); + const later = await refreshIfDue(auth, store); + + expect(later.token).toBe('qc_later'); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(warn).not.toHaveBeenCalled(); + }); + it('does not retry after a definite 4xx', async () => { fetchMock.mockResolvedValue(jsonResponse(400, { error: 'invalid_grant' })); jest.spyOn(console, 'warn').mockImplementation(() => {}); @@ -191,17 +249,23 @@ describe('refreshIfDue', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); - it('waits the recovery delay before the retry', async () => { - recovery.delayMs = 200; - const times: number[] = []; + it('waits the recovery delay after the previous attempt ended', async () => { + recovery.delayMs = 100; + const starts: number[] = []; + const ends: number[] = []; fetchMock.mockImplementation(async () => { - times.push(Date.now()); - return times.length === 1 ? jsonResponse(502, {}) : ok('late'); + starts.push(Date.now()); + await new Promise((resolve) => setTimeout(resolve, 60)); + ends.push(Date.now()); + return starts.length < 3 ? jsonResponse(504, {}) : ok('late'); }); - await refreshIfDue(login(DAY), { save, reload, lockFile, location: 'creds' }); + const result = await refreshIfDue(login(DAY), { save, reload, lockFile, location: 'creds' }); - expect(times[1] - times[0]).toBeGreaterThanOrEqual(190); + expect(result.token).toBe('qc_late'); + expect(starts).toHaveLength(3); + expect(starts[1] - ends[0]).toBeGreaterThanOrEqual(95); + expect(starts[2] - ends[1]).toBeGreaterThanOrEqual(95); }); }); }); diff --git a/src/utils/file-lock.ts b/src/utils/file-lock.ts index 41cfff0..82f5e48 100644 --- a/src/utils/file-lock.ts +++ b/src/utils/file-lock.ts @@ -8,7 +8,12 @@ import { randomBytes } from 'crypto'; export const LOCK_STALE_MS = 90_000; const POLL_MS = 100; -const MAX_WAIT_MS = 2 * LOCK_STALE_MS; +/** + * A refresh with recovery retries can hold the lock for about 5 minutes (the + * last retry starts within 4 minutes and has a 60 s timeout), so a waiter waits + * longer than that. A dead holder is still taken over after LOCK_STALE_MS. + */ +const MAX_WAIT_MS = 6 * 60_000; const DEFERRED_SIGNALS: NodeJS.Signals[] = ['SIGINT', 'SIGTERM']; /** What a holder writes into the lock file. `time` is the holder's own clock. */ @@ -190,14 +195,26 @@ function guardProcess(lockFile: string, nonce: string): () => void { } } }; - const onSignal = (signal: NodeJS.Signals) => { - pending.push(signal); + const removeListeners = () => { + process.off('exit', onExit); + DEFERRED_SIGNALS.forEach((signal) => process.off(signal, onSignal)); }; + // The first signal waits for the locked work. A second one stops at once. + // A process killed by a signal emits no 'exit', so the lock is removed first. + function onSignal(signal: NodeJS.Signals) { + if (pending.length > 0) { + removeListeners(); + onExit(); + process.kill(process.pid, signal); + return; + } + pending.push(signal); + process.stderr.write('Finishing the Quant login update. Press Ctrl-C again to stop now.\n'); + } process.on('exit', onExit); DEFERRED_SIGNALS.forEach((signal) => process.on(signal, onSignal)); return () => { - process.off('exit', onExit); - DEFERRED_SIGNALS.forEach((signal) => process.off(signal, onSignal)); + removeListeners(); if (pending.length > 0) { process.kill(process.pid, pending[0]); } diff --git a/src/utils/token-refresh.ts b/src/utils/token-refresh.ts index d0e1d69..2ff6904 100644 --- a/src/utils/token-refresh.ts +++ b/src/utils/token-refresh.ts @@ -18,11 +18,14 @@ export const REFRESH_WINDOW_MS = 7 * 24 * 60 * 60 * 1000; export const REFRESH_TIMEOUT_MS = 60_000; /** - * After an unknown outcome, qc waits this long and sends the same refresh token - * once more. The portal recovers a rotation whose answer was lost. Tests may - * lower it. + * After an unknown outcome, qc sends the same refresh token again. The portal + * answers the previous token with "just rotated" for 30 s after a rotation, and + * recovers a lost rotation (a new pair, once) from 30 s to 5 minutes. So qc + * waits delayMs after the previous attempt ended, sends at most maxRetries + * retries, and sends none when windowMs has passed since the first send. + * Tests may lower these values. */ -export const recovery = { delayMs: 20_000 }; +export const recovery = { delayMs: 45_000, maxRetries: 2, windowMs: 4 * 60_000 }; /** After this long, qc tells the user that it is still renewing the login. */ const SLOW_NOTICE_MS = 5_000; @@ -95,10 +98,18 @@ interface TokenErrorBody { error_description?: string; } +/** "Just rotated": a request with this token rotated less than 30 s ago, maybe our own. */ +function isJustRotated(body: TokenErrorBody): boolean { + return body.error === 'invalid_grant' && /just rotated/i.test(body.error_description ?? ''); +} + function refreshFailure(status: number, body: TokenErrorBody): Error { if (status >= 500 || (status >= 200 && status < 300)) { return new UnknownRefreshOutcome(`HTTP ${status} without a usable token response`); } + if (isJustRotated(body)) { + return new UnknownRefreshOutcome(body.error_description as string); + } if (body.error) { return new TokenRefreshError(body.error, body.error_description); } @@ -179,32 +190,68 @@ async function refreshLocked( return latest ?? auth; } await lock.verify(); - const updates = await refreshWithRecovery(latest, lock); + const updates = await refreshWithRecovery(latest, store, lock); await saveRotation(store, updates); logger.debug('Access token refreshed'); return { ...latest, ...updates }; }); } +const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +function tokenFields(auth: AuthConfig): Partial { + return { token: auth.token, refreshToken: auth.refreshToken, expiresAt: auth.expiresAt }; +} + /** - * requestRefresh, sent once more with the same refresh token after an unknown - * outcome. The portal answers a lost rotation with a new pair. Never more than - * one retry, and never after a definite 4xx answer. The lock is kept fresh, - * because both attempts and the wait take longer than LOCK_STALE_MS. + * Waits before a recovery retry, with the lock kept fresh. Returns the stored + * tokens when they changed during the wait (the retry is then not sent), or + * null to send the retry. Throws lastError when the window has passed. */ -async function refreshWithRecovery(auth: AuthConfig, lock: HeldLock): Promise> { - try { - return await requestRefresh(auth); - } catch (error) { - if (!(error instanceof UnknownRefreshOutcome)) { - throw error; +async function prepareRetry( + auth: AuthConfig, + store: RefreshStore, + lock: HeldLock, + firstSend: number, + lastError: unknown, +): Promise | null> { + await lock.touch(); + await sleep(recovery.delayMs); + await lock.touch(); + if (Date.now() - firstSend > recovery.windowMs) { + throw lastError; + } + const latest = await store.reload(); + if (!latest) { + throw lastError; + } + return latest.token !== auth.token ? tokenFields(latest) : null; +} + +/** + * requestRefresh, sent again with the same refresh token after an unknown + * outcome (see `recovery`). A definite 4xx is never retried. Both waits and all + * attempts take longer than LOCK_STALE_MS, so the lock is touched around each wait. + */ +async function refreshWithRecovery(auth: AuthConfig, store: RefreshStore, lock: HeldLock): Promise> { + const firstSend = Date.now(); + let lastError: unknown; + for (let attempt = 0; attempt <= recovery.maxRetries; attempt++) { + const stored = attempt > 0 ? await prepareRetry(auth, store, lock, firstSend, lastError) : null; + if (stored) { + return stored; + } + try { + return await requestRefresh(auth); + } catch (error) { + if (!(error instanceof UnknownRefreshOutcome)) { + throw error; + } + lastError = error; + logger.debug(`Refresh outcome unknown (${error.message})`); } - logger.debug(`Refresh outcome unknown (${error.message}); retrying once in ${recovery.delayMs} ms`); - await lock.touch(); - await new Promise((resolve) => setTimeout(resolve, recovery.delayMs)); - await lock.touch(); - return requestRefresh(auth); } + throw lastError; } /** From 4f6673212c3d76a7898be71b3d1491d99e1a8747 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Thu, 5 Nov 2026 20:55:08 +1000 Subject: [PATCH 13/15] release: 0.9.0 (OAuth refresh tokens) --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 059c732..f11868f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@quantcdn/quant-cloud-cli", - "version": "0.8.0", + "version": "0.9.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@quantcdn/quant-cloud-cli", - "version": "0.8.0", + "version": "0.9.0", "license": "MIT", "dependencies": { "@quantcdn/quant-client": "^4.30.0", diff --git a/package.json b/package.json index 4cb8231..8f3ebd5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@quantcdn/quant-cloud-cli", - "version": "0.8.0", + "version": "0.9.0", "description": "Command-line interface for Quant Cloud Platform integration and management", "type": "module", "main": "dist/index.js", From 506faad19342d9d2567f5ecea5dcc5b908f7e95e Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Fri, 6 Nov 2026 18:17:23 +1000 Subject: [PATCH 14/15] fix(auth): tell the user why a refresh recovery waits 45 s Found in the local e2e run: a dropped refresh response made qc wait 46 s with no output. --- src/utils/token-refresh.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/utils/token-refresh.ts b/src/utils/token-refresh.ts index 2ff6904..5046891 100644 --- a/src/utils/token-refresh.ts +++ b/src/utils/token-refresh.ts @@ -216,6 +216,7 @@ async function prepareRetry( lastError: unknown, ): Promise | null> { await lock.touch(); + process.stderr.write(`Renewing Quant login: no clear answer from the server. Trying again in ${Math.round(recovery.delayMs / 1000)} s…\n`); await sleep(recovery.delayMs); await lock.touch(); if (Date.now() - firstSend > recovery.windowMs) { From eb4e2424066b60fcda49e84e1ee8056e1cd82a22 Mon Sep 17 00:00:00 2001 From: Stuart Rowlands Date: Fri, 6 Nov 2026 20:45:05 +1000 Subject: [PATCH 15/15] fix(auth): the wait notice does not promise a retry Review of 506faad: after the wait, the window may have passed or another process may have rotated the token, so no retry is sent. --- src/utils/token-refresh.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/token-refresh.ts b/src/utils/token-refresh.ts index 5046891..7d99944 100644 --- a/src/utils/token-refresh.ts +++ b/src/utils/token-refresh.ts @@ -216,7 +216,7 @@ async function prepareRetry( lastError: unknown, ): Promise | null> { await lock.touch(); - process.stderr.write(`Renewing Quant login: no clear answer from the server. Trying again in ${Math.round(recovery.delayMs / 1000)} s…\n`); + process.stderr.write(`Renewing Quant login: no clear answer from the server. Waiting ${Math.round(recovery.delayMs / 1000)} s…\n`); await sleep(recovery.delayMs); await lock.touch(); if (Date.now() - firstSend > recovery.windowMs) {