diff --git a/src/api/ctClient.ts b/src/api/ctClient.ts index 3a06f21..d6a135f 100644 --- a/src/api/ctClient.ts +++ b/src/api/ctClient.ts @@ -14,6 +14,7 @@ * here can be swapped for `openapi-fetch` while keeping this class's surface. */ import { resolveConfig, type CtConfig } from "../config.js"; +import { fetchWithRetry } from "./http.js"; export interface WhoAmI { id: number; @@ -48,7 +49,11 @@ export class CtClient { /** Run the login-token handshake and cache the session cookie + CSRF token. */ async authenticate(loginToken: string): Promise { const url = `${this.config.host}/api/whoami?login_token=${encodeURIComponent(loginToken)}`; - const res = await fetch(url, { headers: { Accept: "application/json" } }); + const res = await fetchWithRetry( + url, + { headers: { Accept: "application/json" } }, + { isIdempotent: true }, + ); this.captureCookie(res); if (!res.ok) { throw new CtApiError(`Login failed (whoami)`, res.status, await safeBody(res)); @@ -82,11 +87,15 @@ export class CtClient { if (body !== undefined) { headers["Content-Type"] = "application/json"; } - const res = await fetch(`${this.config.host}/api${path}`, { - method, - headers, - body: body !== undefined ? JSON.stringify(body) : undefined, - }); + const res = await fetchWithRetry( + `${this.config.host}/api${path}`, + { + method, + headers, + body: body !== undefined ? JSON.stringify(body) : undefined, + }, + { isIdempotent: method === "GET" || method === "HEAD" }, + ); this.captureCookie(res); if (!res.ok) { throw new CtApiError(`${method} ${path} failed`, res.status, await safeBody(res)); @@ -102,9 +111,11 @@ export class CtClient { if (!this.cookie) { return; } - const res = await fetch(`${this.config.host}/api/csrftoken`, { - headers: { Accept: "application/json", Cookie: this.cookie }, - }); + const res = await fetchWithRetry( + `${this.config.host}/api/csrftoken`, + { headers: { Accept: "application/json", Cookie: this.cookie } }, + { isIdempotent: true }, + ); this.captureCookie(res); if (!res.ok) { throw new CtApiError("Failed to fetch CSRF token", res.status, await safeBody(res)); diff --git a/src/api/http.ts b/src/api/http.ts new file mode 100644 index 0000000..4ccf99e --- /dev/null +++ b/src/api/http.ts @@ -0,0 +1,102 @@ +/** + * HTTP with rate-limit awareness and bounded retry. + * + * ChurchTools rate-limits bursts (HTTP 429) and can return transient 5xx. This + * wrapper retries with exponential backoff + jitter, honouring a `Retry-After` + * header when present. + * + * Safety: only **idempotent** requests (GET/HEAD) are retried on 5xx or network + * errors — a write that may have already been applied is never blindly repeated. + * A 429 is always safe to retry: the server rejected the request before + * processing it. + */ + +export interface RetryOptions { + /** Max additional attempts after the first (total attempts = retries + 1). */ + retries?: number; + baseDelayMs?: number; + /** GET/HEAD are idempotent; writes are not. Controls 5xx/network retry. */ + isIdempotent?: boolean; + sleep?: (ms: number) => Promise; + fetchImpl?: typeof fetch; +} + +/** Cap on any single wait, so an outsized `Retry-After` can't hang the CLI. */ +const MAX_DELAY_MS = 60_000; + +const defaultSleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +function clampDelay(ms: number): number { + if (!Number.isFinite(ms)) { + return 0; + } + return Math.min(Math.max(ms, 0), MAX_DELAY_MS); +} + +function backoffMs(base: number, attempt: number): number { + const exp = base * 2 ** (attempt - 1); + return clampDelay(exp + Math.floor(Math.random() * base)); +} + +function retryAfterMs(res: Response): number | null { + const header = res.headers.get("retry-after")?.trim(); + if (!header) { + return null; + } + // `Retry-After` is either a non-negative delta-seconds count or an HTTP-date. + if (/^\d+$/.test(header)) { + return clampDelay(Number.parseInt(header, 10) * 1000); + } + // Only treat it as a date when it actually looks like one — `Date.parse` is + // lax enough to accept e.g. "-5" as a year, which must not become a 0ms wait. + if (/[a-zA-Z]/.test(header)) { + const dateMs = Date.parse(header); + if (Number.isFinite(dateMs)) { + return clampDelay(dateMs - Date.now()); + } + } + // Unparseable (e.g. a negative or malformed value): fall back to backoff. + return null; +} + +function shouldRetryStatus(status: number, isIdempotent: boolean): boolean { + if (status === 429) { + return true; + } + return status >= 500 && isIdempotent; +} + +export async function fetchWithRetry( + input: string | URL, + init: RequestInit, + opts: RetryOptions = {}, +): Promise { + const retries = opts.retries ?? 3; + const base = opts.baseDelayMs ?? 500; + const isIdempotent = opts.isIdempotent ?? true; + const sleep = opts.sleep ?? defaultSleep; + const doFetch = opts.fetchImpl ?? fetch; + + let attempt = 0; + for (;;) { + attempt++; + let res: Response; + try { + res = await doFetch(input, init); + } catch (err) { + if (attempt <= retries && isIdempotent) { + await sleep(backoffMs(base, attempt)); + continue; + } + throw err; + } + if (attempt <= retries && shouldRetryStatus(res.status, isIdempotent)) { + const delay = retryAfterMs(res) ?? backoffMs(base, attempt); + // Drain the response we're discarding so its socket isn't left buffered. + await res.body?.cancel().catch(() => {}); + await sleep(delay); + continue; + } + return res; + } +} diff --git a/src/api/session.ts b/src/api/session.ts index 2c93c5a..ebd2c8f 100644 --- a/src/api/session.ts +++ b/src/api/session.ts @@ -4,6 +4,7 @@ */ import { CtClient, type WhoAmI } from "./ctClient.js"; import { readToken } from "../auth/tokenStore.js"; +import { resolveConfig } from "../config.js"; export interface AuthedSession { client: CtClient; @@ -11,11 +12,12 @@ export interface AuthedSession { } export async function authedSession(): Promise { + const config = resolveConfig(); const token = await readToken(); if (!token) { throw new Error("Not logged in. Run `ct auth login --token ` first."); } - const client = new CtClient(); + const client = new CtClient(config); const me = await client.authenticate(token); return { client, me }; } diff --git a/src/auth/tokenStore.ts b/src/auth/tokenStore.ts index 3473d44..812d500 100644 --- a/src/auth/tokenStore.ts +++ b/src/auth/tokenStore.ts @@ -1,40 +1,75 @@ /** * Persistence for the personal ChurchTools login token. * - * Precedence when reading: + * A single token is stored in the macOS Keychain (via the `security` CLI). + * There is no file fallback: on CI or non-macOS hosts, supply the token through + * the `CT_LOGINTOKEN` environment variable instead. + * + * Read precedence: * 1. `CT_LOGINTOKEN` environment variable (CI / one-off use) - * 2. credentials file at `~/.config/ct-cli/credentials.json` + * 2. macOS Keychain * - * TODO(Phase 1, #3): move the file store behind the macOS Keychain (e.g. - * `security add-generic-password`) and keep the file only as a non-macOS - * fallback. The interface below stays the same so callers don't change. + * Note: `security ... -w ` passes the token as an argv, briefly visible + * to `ps`. Acceptable for a local developer CLI; the value never touches git. */ -import { homedir } from "node:os"; -import { join } from "node:path"; -import { mkdir, readFile, writeFile, rm, chmod } from "node:fs/promises"; +import { platform } from "node:os"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; + +const run = promisify(execFile); +const KEYCHAIN_SERVICE = "ct-cli"; +const KEYCHAIN_ACCOUNT = "login-token"; + +function isMac(): boolean { + return platform() === "darwin"; +} -interface Credentials { - host: string; - token: string; +async function keychainSet(token: string): Promise { + await run("security", [ + "add-generic-password", + "-U", + "-s", + KEYCHAIN_SERVICE, + "-a", + KEYCHAIN_ACCOUNT, + "-w", + token, + ]); } -function configDir(): string { - const base = process.env.XDG_CONFIG_HOME?.trim() || join(homedir(), ".config"); - return join(base, "ct-cli"); +async function keychainGet(): Promise { + try { + const { stdout } = await run("security", [ + "find-generic-password", + "-s", + KEYCHAIN_SERVICE, + "-a", + KEYCHAIN_ACCOUNT, + "-w", + ]); + return stdout.trim() || null; + } catch { + return null; + } } -function credentialsPath(): string { - return join(configDir(), "credentials.json"); +async function keychainDelete(): Promise { + try { + await run("security", ["delete-generic-password", "-s", KEYCHAIN_SERVICE, "-a", KEYCHAIN_ACCOUNT]); + } catch { + /* not present — nothing to delete */ + } } -export async function storeToken(host: string, token: string): Promise { - const dir = configDir(); - await mkdir(dir, { recursive: true }); - const path = credentialsPath(); - const payload: Credentials = { host, token }; - await writeFile(path, `${JSON.stringify(payload, null, 2)}\n`, { mode: 0o600 }); - await chmod(path, 0o600); - return path; +/** Persist the token in the macOS Keychain; returns a human-readable location. */ +export async function storeToken(token: string): Promise { + if (!isMac()) { + throw new Error( + "Token storage requires the macOS Keychain. On other platforms, set CT_LOGINTOKEN instead.", + ); + } + await keychainSet(token); + return `macOS Keychain (service "${KEYCHAIN_SERVICE}", account "${KEYCHAIN_ACCOUNT}")`; } export async function readToken(): Promise { @@ -42,15 +77,14 @@ export async function readToken(): Promise { if (fromEnv) { return fromEnv; } - try { - const raw = await readFile(credentialsPath(), "utf8"); - const parsed = JSON.parse(raw) as Partial; - return parsed.token?.trim() || null; - } catch { - return null; + if (isMac()) { + return keychainGet(); } + return null; } export async function clearToken(): Promise { - await rm(credentialsPath(), { force: true }); + if (isMac()) { + await keychainDelete(); + } } diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 66a7520..6e1aac1 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -23,9 +23,9 @@ export function authCommand(): Command { } const client = new CtClient(config); const me = await client.authenticate(token); - const path = await storeToken(config.host, token); + const location = await storeToken(token); success(`Logged in to ${config.host} as ${me.firstName ?? ""} ${me.lastName ?? ""} (#${me.id})`.trim()); - info(`Token stored at ${path} (mode 0600).`); + info(`Token stored in ${location}.`); const ctInfo = await client.get("/info"); if (ctInfo.version) { diff --git a/tests/http.test.ts b/tests/http.test.ts new file mode 100644 index 0000000..fa0565a --- /dev/null +++ b/tests/http.test.ts @@ -0,0 +1,90 @@ +import { describe, it, expect, vi } from "vitest"; +import { fetchWithRetry } from "../src/api/http.js"; + +const noSleep = () => Promise.resolve(); + +function res(status: number, headers: Record = {}): Response { + return new Response(status === 204 ? null : "{}", { status, headers }); +} + +describe("fetchWithRetry", () => { + it("retries a 429 then succeeds", async () => { + const fetchImpl = vi.fn().mockResolvedValueOnce(res(429)).mockResolvedValueOnce(res(200)); + const out = await fetchWithRetry("https://x/api/campuses", {}, { sleep: noSleep, fetchImpl }); + expect(out.status).toBe(200); + expect(fetchImpl).toHaveBeenCalledTimes(2); + }); + + it("honours Retry-After for the wait duration", async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(res(429, { "retry-after": "2" })) + .mockResolvedValueOnce(res(200)); + const sleep = vi.fn(() => Promise.resolve()); + await fetchWithRetry("https://x/api/campuses", {}, { sleep, fetchImpl }); + expect(sleep).toHaveBeenCalledWith(2000); + }); + + it("caps an outsized Retry-After so the CLI can't hang for hours", async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(res(429, { "retry-after": "86400" })) + .mockResolvedValueOnce(res(200)); + const sleep = vi.fn(() => Promise.resolve()); + await fetchWithRetry("https://x/api/campuses", {}, { sleep, fetchImpl }); + expect(sleep).toHaveBeenCalledWith(60_000); + }); + + it("ignores a malformed Retry-After and falls back to backoff", async () => { + const fetchImpl = vi + .fn() + .mockResolvedValueOnce(res(429, { "retry-after": "-5" })) + .mockResolvedValueOnce(res(200)); + const sleep = vi.fn<(ms: number) => Promise>(() => Promise.resolve()); + await fetchWithRetry("https://x/api/campuses", {}, { baseDelayMs: 500, sleep, fetchImpl }); + // Backoff, not an immediate (negative → 0) retry. + expect(sleep).toHaveBeenCalledTimes(1); + expect(sleep.mock.calls[0]?.[0] ?? 0).toBeGreaterThanOrEqual(500); + }); + + it("retries idempotent GET on 500", async () => { + const fetchImpl = vi.fn().mockResolvedValueOnce(res(500)).mockResolvedValueOnce(res(200)); + const out = await fetchWithRetry( + "https://x/api/campuses", + {}, + { sleep: noSleep, fetchImpl, isIdempotent: true }, + ); + expect(out.status).toBe(200); + expect(fetchImpl).toHaveBeenCalledTimes(2); + }); + + it("does NOT retry a write on 500", async () => { + const fetchImpl = vi.fn().mockResolvedValueOnce(res(500)); + const out = await fetchWithRetry( + "https://x/api/campuses", + { method: "POST" }, + { sleep: noSleep, fetchImpl, isIdempotent: false }, + ); + expect(out.status).toBe(500); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); + + it("gives up after the retry budget and returns the last response", async () => { + const fetchImpl = vi.fn().mockResolvedValue(res(429)); + const out = await fetchWithRetry("https://x/api/campuses", {}, { retries: 2, sleep: noSleep, fetchImpl }); + expect(out.status).toBe(429); + expect(fetchImpl).toHaveBeenCalledTimes(3); + }); + + it("does not retry a write on a network error", async () => { + const fetchImpl = vi.fn().mockRejectedValue(new Error("ECONNRESET")); + await expect( + fetchWithRetry( + "https://x/api/campuses", + { method: "POST" }, + { sleep: noSleep, fetchImpl, isIdempotent: false }, + ), + ).rejects.toThrow("ECONNRESET"); + expect(fetchImpl).toHaveBeenCalledTimes(1); + }); +}); diff --git a/tests/tokenStore.test.ts b/tests/tokenStore.test.ts new file mode 100644 index 0000000..61711c0 --- /dev/null +++ b/tests/tokenStore.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { readToken } from "../src/auth/tokenStore.js"; + +const original = process.env.CT_LOGINTOKEN; + +afterEach(() => { + if (original === undefined) { + delete process.env.CT_LOGINTOKEN; + } else { + process.env.CT_LOGINTOKEN = original; + } +}); + +describe("readToken", () => { + it("prefers the CT_LOGINTOKEN env var over any store", async () => { + process.env.CT_LOGINTOKEN = "env-token"; + await expect(readToken()).resolves.toBe("env-token"); + }); +});