diff --git a/devlog/_plan/260909_xai_oauth_retry_hardening/000_plan.md b/devlog/_plan/260909_xai_oauth_retry_hardening/000_plan.md new file mode 100644 index 0000000000..785622752e --- /dev/null +++ b/devlog/_plan/260909_xai_oauth_retry_hardening/000_plan.md @@ -0,0 +1,101 @@ +# xAI OAuth retry hardening — unit plan + +## Reader summary + +Four filed defects in `src/oauth/xai.ts` (#4045, #4046, #4047, #4048) all sit in +the token-request path that runs on every Grok login and every token refresh: +`postXaiToken` clamps the server-provided `Retry-After` to 2 s, ignores its +HTTP-date and fractional forms, and keeps retrying after the caller has aborted +whenever the abort carries a custom reason; `validateXaiEndpoint` accepts any +`*.x.ai` subdomain and URLs with embedded userinfo on the endpoint that receives +the `refresh_token`. This unit ships two pull requests: phase 1 fixes the three +retry/abort bugs as one cohesive change, phase 2 hardens endpoint validation as a +separate security change layered on phase 1. Grok-account users get retries that +honor the server and stop when cancelled; the credential-destination check stops +trusting unbounded subdomains. + +## Loop spec + +- **Loop archetype:** satisfy-spec — the expected contracts are stated in the four + public issues; no candidate exploration. +- **Trigger:** delegated release-preparation task from the managing thread + (`01a08498-4ebf-7ad3-89c2-fb56c8ea53bf`), user-authorized: implement, publish + PRs, verify via remote CI, await serial merge assignment. +- **Goal:** PR1 (base `dev`) fixes #4045/#4046/#4047 with regression tests; PR2 + (base = PR1 head) fixes #4048 with regression tests; both carry exact-head + remote CI evidence. +- **Non-goals:** release, promotion, `main` push, deploy, merges (merge awaits + the managing task's serial assignment); no local product test + suite/typecheck/build/install (user restriction — labeled NOT RUN); no changes + to other providers' auth lanes; no GitHub native stacks (ordinary dependent PR + chain only). +- **Verifier:** GitHub Actions `ci.yml` ("Cross-platform CI") on each pull + request — it triggers on every `pull_request` event with no base filter and + its `changes` gate includes `src/**` and `tests/**`, so both PRs (including + the stacked child) receive the real PR jobs: Linux `test`, macOS + `platform-macos`, and `gates` (`tsc --noEmit`). The Windows + `platform-windows` and `macos-control` jobs are NOT ON PR — they run only + via `workflow_dispatch` `lane=all`; the cumulative final-head `lane=all` + dispatch before any merge is owned by the managing task. Exact-head check runs + are recorded per PR. Local verifiers (`bun test`, `bun run typecheck`) are + NOT RUN by user restriction; the plan instead maps each new test to the file CI + executes (`tests/providers/xai/xai-oauth-retry.test.ts`). +- **Stop condition:** both PRs published with template-complete bodies, + exact-head CI recorded, and the managing task handed the PR/head/CI report. +- **Memory artifact:** this unit directory; goalplan + `.codexclaw/goalplans/fix-opencodex-4045-4046-4047-xai-oauth-retry-aft/`; + scratch-only security detail in `.tmp/260909_xai_endpoint_security/` + (gitignored) per the repository security-notes policy. +- **Expected terminal outcomes:** DONE = both PRs published with green exact-head + CI and handoff reported. BLOCKED = CI failure that requires out-of-scope files, + or a policy question only the managing task can answer. +- **Escalation condition:** any need to touch files outside the owned set + (`src/oauth/xai.ts`, `tests/providers/xai/`, this unit, scratch), any merge or + `main`/`preview` action, or a verifier verdict that contradicts the issue + contract. A delegated slice that two agents fail returns to the main lane rather + than being re-dispatched. + +### HOTL resource bounds + +- Tool/credential scope: `gh` as the repository owner for reads and PR creation + on `lidge-jun/opencodex`; no merge, release, or settings writes. +- Write scope: `src/oauth/xai.ts`, `tests/providers/xai/xai-oauth-retry.test.ts`, + `devlog/_plan/260909_xai_oauth_retry_hardening/`, `.tmp/` scratch. Git + mutations use `-c core.hooksPath=/dev/null`; pushes use `--no-verify`. +- Token/cost and wall-clock budgets: none set by the user; unbounded within the + session, reported at handoff. + +## Constraints + +- `src/AGENTS.md`: OAuth/token changes are security-boundary changes; regression + coverage sits near the existing subsystem tests; public exports are preserved. +- `tests/providers/xai/xai-oauth-retry.test.ts` already exists in + `scripts/test-layout/layout.json` `explicit` (line 1301) and + `tests/fixtures/test-layout-expected.json`; extending it avoids layout churn. +- Root `AGENTS.md`: pre-disclosure security working notes live in scratch + (`.tmp/`), never in `devlog/`. Issue #4048 is public and its sketch fix is + public, but the assessment and patch plan for phase 2 are held in scratch until + the PR diff itself is public. +- `MAINTAINERS.md` (2026-09-06): maintainer integration into `dev` without a + second approval exists but is exercised only by the managing task; this unit + performs no merges. + +## Work-phase map (dependency-ordered) + +| Phase | Doc | Output | Depends on | +|-------|-----|--------|------------| +| wp1 (this cycle) | `000_plan.md`, `010_*`, `020_*` | Locked roadmap | — | +| wp2 | `010_phase1_retry_after_abort.md` | PR1: retry/abort fixes + tests, base `dev` | wp1 | +| wp3 | `020_phase2_endpoint_validation.md` | PR2: endpoint validation hardening + tests, base = PR1 head | wp2 (same file; layered to avoid self-conflict) | + +Phase order follows the build order: the retry-path repair rewrites the same +function cluster the endpoint guard sits next to, so the security layer stacks on +the repaired file rather than racing it as a parallel root. + +## Independent verification + +Four read-only verifier subagents (xai/grok-4.6, one per issue) re-check each +claimed defect and proposed fix against the live code before phase 1 builds; their +verdicts fold into the phase-1 audit. Live discovery evidence (2026-09-09): +`https://auth.x.ai/.well-known/openid-configuration` returns only +`auth.x.ai` hosts for `authorization_endpoint` and `token_endpoint`. diff --git a/devlog/_plan/260909_xai_oauth_retry_hardening/010_phase1_retry_after_abort.md b/devlog/_plan/260909_xai_oauth_retry_hardening/010_phase1_retry_after_abort.md new file mode 100644 index 0000000000..7524598fe4 --- /dev/null +++ b/devlog/_plan/260909_xai_oauth_retry_hardening/010_phase1_retry_after_abort.md @@ -0,0 +1,351 @@ +# Phase 1 (wp2): honor Retry-After, stop retrying aborted token requests + +Closes #4045, #4046, #4047. One cohesive PR: all three defects live in the same +retry loop of `postXaiToken` and the same helper cluster; splitting them would +produce three PRs editing adjacent lines of one function. + +Revision 3, folding two audit rounds. Round 1: four issue verifiers (#4045 +CONFIRMED, #4046 PARTIALLY, #4047 PARTIALLY, #4048 CONFIRMED) — strict parser +copied from `src/combos/failover.ts`, two-name terminal guard, pre-sleep abort +check. Round 2: plan auditor GO-WITH-FIXES (blockers=1) plus a bounded +retry-algorithm reviewer FAIL (one real High) — folded below: donor-fidelity +date regex, non-vacuous HTTP-date test, hostile-vector tests, corrected CI map, +**abort-aware in-wait sleep**, and the **retry-budget terminal rule** replacing +the issue sketch's silent 60 s clamp. + +## File change map + +| Path | Action | What | +|------|--------|------| +| `src/oauth/xai.ts` | MODIFY | import `abortError`/`sleepWithAbort` from `../lib/upstream-retry`; `retryDelay` rewrite (returns `number \| undefined`), new `jitterDelay`/`parseRetryAfterMs`/`parseHttpDateMs`/`sleepAbortable`, remove `isAbortError`, terminal abort/timeout handling + abort-aware backoff in `postXaiToken` | +| `tests/providers/xai/xai-oauth-retry.test.ts` | MODIFY | new regression tests (below); existing five must keep passing unmodified | +| `docs-site/` | none | retry timing is internal; no user-facing configuration or documented behavior changes | + +`src/lib/upstream-retry.ts` is a documented leaf module (its header: "MUST stay +a leaf module") importing only `./abort`, so the new import adds no transitive +weight to the OAuth path and reuses the repo-standard `abortError` shape +(`signal.reason ?? DOMException("The operation was aborted", "AbortError")`). + +Scope boundary — IN: the two rows above plus this unit directory. OUT: every other +provider's OAuth lane, `callback-server.ts`, `pkce.ts`, `validateXaiEndpoint` +(phase 2), `src/combos/failover.ts` (parser donor — copied, not imported), +CLI surfaces, GUI. + +## Diff-level design + +### 1. Constants (NEW, next to `TOKEN_REQUEST_TIMEOUT_MS` at src/oauth/xai.ts:13) + +```ts +const RETRY_AFTER_MAX_DELAY_MS = 60_000; +const JITTER_DELAY_CAP_MS = 2_000; +``` + +### 2. Parser and delay helpers (NEW/REWRITE) + +Before (src/oauth/xai.ts:98, current `dev`): + +```ts +function retryDelay(attempt:number,retryAfter:string|null,random:()=>number):number{const base=attempt===1?100:250,j=Math.round(base*(.75+random()*.5)),seconds=retryAfter!==null&&/^\d+$/.test(retryAfter)?Number(retryAfter):0;return Math.min(2000,Math.max(j,seconds*1000));} +``` + +After: + +```ts +const IMF_FIXDATE_RE = /^(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), (\d{2}) (Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) (\d{4}) (\d{2}):(\d{2}):(\d{2}) GMT$/i; +const HTTP_MONTH_INDEX: Record = { + jan: 0, feb: 1, mar: 2, apr: 3, may: 4, jun: 5, + jul: 6, aug: 7, sep: 8, oct: 9, nov: 10, dec: 11, +}; + +function parseHttpDateMs(value: string): number | undefined { + const match = IMF_FIXDATE_RE.exec(value); + if (!match) return undefined; + const month = HTTP_MONTH_INDEX[match[2]!.toLowerCase()]; + if (month === undefined) return undefined; + const year = Number(match[3]); + const day = Number(match[1]); + const hour = Number(match[4]); + const minute = Number(match[5]); + const second = Number(match[6]); + const timestamp = Date.UTC(year, month, day, hour, minute, second); + const parsed = new Date(timestamp); + return parsed.getUTCFullYear() === year + && parsed.getUTCMonth() === month + && parsed.getUTCDate() === day + && parsed.getUTCHours() === hour + && parsed.getUTCMinutes() === minute + && parsed.getUTCSeconds() === second + ? timestamp + : undefined; +} + +function parseRetryAfterMs(retryAfter: string | null): number | undefined { + const text = retryAfter?.trim(); + if (!text) return undefined; + if (/^\d+(?:\.\d+)?$/.test(text)) { + const ms = Math.ceil(Number(text) * 1000); + return ms > 0 ? ms : undefined; + } + const timestamp = parseHttpDateMs(text); + if (timestamp === undefined) return undefined; + const delay = timestamp - Date.now(); + return delay > 0 ? delay : undefined; +} + +function jitterDelay(attempt: number, random: () => number): number { + const base = attempt === 1 ? 100 : 250; + return Math.min(JITTER_DELAY_CAP_MS, Math.round(base * (0.75 + random() * 0.5))); +} + +/** + * Delay before the next attempt, or undefined when the server asked for a wait + * beyond the retry budget — retrying earlier than Retry-After is the original + * #4045 defect shape, so the caller must fail instead of clamping. + */ +function retryDelay(attempt: number, retryAfter: string | null, random: () => number): number | undefined { + const serverMs = parseRetryAfterMs(retryAfter); + if (serverMs === undefined) return jitterDelay(attempt, random); + return serverMs <= RETRY_AFTER_MAX_DELAY_MS ? serverMs : undefined; +} + +async function sleepAbortable( + ms: number, + sleep: (ms: number) => Promise, + signal: AbortSignal | undefined, +): Promise { + if (!signal) return sleep(ms); + if (signal.aborted) throw abortError(signal); + let onAbort!: () => void; + try { + await Promise.race([ + sleep(ms), + new Promise((_, reject) => { + onAbort = () => reject(abortError(signal)); + signal.addEventListener("abort", onAbort, { once: true }); + }), + ]); + } finally { + signal.removeEventListener("abort", onAbort); + } +} +``` + +Default sleep primitive (post-review amendment, verified by direct read of +`src/lib/upstream-retry.ts:53`): the production default changes from +`Bun.sleep` to the already-exported `sleepWithAbort`, which clears its own +timer on abort — so a cancelled CLI leaves no live 60 s timer behind: + +```ts +const sleep = deps.sleep ?? ((ms: number) => sleepWithAbort(ms, signal)); +``` + +`sleepAbortable` remains as the wrapper so a test-injected `deps.sleep` is +still raced against the caller signal; in production the composition is +sleepWithAbort's own abort handling plus the wrapper's reason-preserving +rejection. `upstream-retry.ts` is a documented leaf importing only +`./abort`; no shared-module export or API change is needed. + +Parser provenance (round-1 fold): compact copy of the strict parser the +repository already maintains in `src/combos/failover.ts:117`, at donor fidelity +(round-2 fold): case-insensitive enumerated weekday/month names and a full +six-field UTC round-trip check, so `10:60:00` overflow and lowercase dates +behave exactly as the donor. NOT the issue's `Number()`/`Date.parse` sketch: +bare `Number()` over-accepts (`"1e3"`, `"0x10"`, `"+2"`) and +`Date.parse` is implementation-defined off IMF-fixdate. The donor is not +imported because that would couple `src/oauth/` to `src/combos/`; a +`src/lib/` unification is a possible follow-up, out of scope here. HTTP-date +support covers ALL THREE RFC 9110 formats at full donor fidelity (round-3 fold +of a CodeRabbit Major): IMF-fixdate, RFC 850 (including the two-digit-year +50-year rule), and asctime — RFC 9110 §5.6.7 requires a recipient parsing an +HTTP-date to accept all three formats; only senders are confined to +IMF-fixdate. An earlier draft of this plan claimed recipients need only parse +IMF-fixdate and was wrong. Fractional seconds are a repo-local interop +extension already honored at `failover.ts:124`. Zero, negative, past-dated, +and unparseable values fall back to jitter. + +Contract changes, all intentional: + +- Server-provided delays up to `RETRY_AFTER_MAX_DELAY_MS` (60 s) are honored + exactly instead of being clamped to 2 s (#4045). The 2 s cap now applies to + the jittered fallback only. +- **Retry-budget terminal rule (round-2 fold, supersedes the issue sketch's + `Math.min(seconds*1000, 60_000)`):** a server delay ABOVE the 60 s budget + (`Retry-After: 61`, `3600`, a far-future date) makes the attempt terminal — + the 429/5xx error is thrown immediately with zero further fetches. Clamping + to 60 s would retry earlier than the server asked, recreating the original + defect; the local retry budget cannot honor that floor, so it stops instead. +- In-wait cancellation (round-2 fold): the backoff sleep is raced against the + caller signal with listener cleanup, so an abort DURING a honored 60 s wait + rejects promptly with the abort reason instead of up to ~120 s late + (`Bun.sleep` is not signal-aware). The test-injected `deps.sleep` primitive + is preserved — the wrapper races whatever sleep is injected. +- The old `Math.max(jitter, serverMs)` floor is dropped: a present server + value wins outright; jitter exists only for the no-header case. +- Ceiling interplay: two honored 60 s waits can stretch wall-clock to ~120 s + while `TOKEN_REQUEST_TIMEOUT_MS` stays 30 s per attempt; per-attempt fetch + timeout is unchanged. An abort during any wait now cancels promptly. + +### 3. Abort/timeout terminal handling in `postXaiToken` (MODIFY) + +Catch branch — before (src/oauth/xai.ts:114 area): + +```ts +}catch(error){if(isAbortError(error)&&signal?.aborted)throw error;last=error; +``` + +After (only the abort predicate and the sleep change; the attempt-3 wrap and +continue are kept verbatim): + +```ts +} catch (error) { + if (signal?.aborted) throw error; + const name = (error as { name?: string } | undefined)?.name; + if (name === "AbortError" || name === "TimeoutError") throw error; + last = error; + if (attempt === 3) { + throw new XaiTokenRequestError(undefined, undefined, "xAI token request failed: network error", { cause: error }); + } + await sleepAbortable(jitterDelay(attempt, random), sleep, signal); + continue; +``` + +Response branch — after: + +```ts + const error = await readTokenError(response); + last = error; + if (!(response.status === 429 || response.status >= 500) || attempt === 3) throw error; + if (signal?.aborted) throw error; + const delay = retryDelay(attempt, response.headers.get("retry-after"), random); + if (delay === undefined) throw error; + await sleepAbortable(delay, sleep, signal); +``` + +`isAbortError` is deleted (definition and only use are both in this file). +Rationale: + +- The class check fails when `controller.abort(reason)` carries a custom reason: + fetch rejects with the reason object as-is, so `instanceof DOMException` is + false and the loop slept and retried an already-aborted request (#4047). + Checking `signal?.aborted` covers every abort reason. +- The internal 30 s `AbortSignal.timeout` in `requestSignal` fires without + aborting the caller's signal; the name guard covers BOTH `AbortError` and + `TimeoutError` because Bun linked-signal timeouts often reject as + `AbortError` (`src/server/images.ts:349`), matching the two-name + non-retryable policy at `src/lib/upstream-retry.ts:178`. The check is safe + here because the fetch signal is always `requestSignal(signal)` — a + composition of exactly the caller signal and the internal timeout — so an + abort-named rejection with a live caller signal can only mean the internal + timeout fired. +- The pre-sleep `signal?.aborted` check in the response branch plus the + abort-raced sleep mean a caller abort is honored before AND during the wait. + +## Regression tests (all in `tests/providers/xai/xai-oauth-retry.test.ts`) + +Existing helpers reused: `queue(...)`, `ok()`, `body`, injected +`{ sleep, random }` deps. All Retry-After cases drive the exported +`postXaiToken` with a 429 response carrying the header — never the unexported +helpers directly. + +1. `429 honors Retry-After seconds beyond the jitter cap` — queue + `[429(retry-after: 60), ok()]`, `random: () => 0.5`; expect sleeps + `[60000]` and 2 fetch calls. Proves #4045. +2. `Retry-After above the 60s budget is terminal, never retried early` — + `retry-after: 3600`; expect rejection with the 429 `XaiTokenRequestError`, + exactly 1 fetch call, zero sleeps. Proves the retry-budget rule (the + anti-#4045 invariant: never retry earlier than the server asked). +3. `Retry-After one second above the budget is terminal` — `retry-after: 61`; + same expectations as test 2. Pins the boundary. +4. `Retry-After below the old 2s cap is still honored exactly` — + `retry-after: 1`; expect `[1000]`. Pins the no-clamp edge. +5. `fractional Retry-After is honored` — `retry-after: 1.5`; expect + `[1500]`. Proves #4046 (fractional). +6. `HTTP-date Retry-After is honored` — header + `new Date(Date.now() + 30_000).toUTCString()`, `random: () => 0.5` pinned; + expect one sleep `> 2000` and `<= 30000` — above the jitter cap, so a + missing or broken `parseHttpDateMs` (which would sleep ~100 ms of jitter) + fails this test. Proves #4046 (HTTP-date). +6b. `RFC 850 HTTP-date Retry-After is honored` — future date formatted + `Wednesday, 09-Sep-26 ... GMT` (two-digit year, 50-year rule); same + `> 2000 && <= 30000` assertion with pinned random. Proves the RFC 850 + recipient form (round-3 fold). +6c. `asctime HTTP-date Retry-After is honored` — future date formatted + `Wed Sep 9 ... 2026` (space-padded day); same assertion. Proves the + asctime recipient form (round-3 fold). +7. `unparseable Retry-After falls back to jitter` — `retry-after: soon`, + `random: () => 0.5`; expect `[100]`. +8. `past HTTP-date falls back to jitter` — `Sun, 06 Nov 1994 08:49:37 GMT`, + `random: () => 0.5`; expect `[100]`. +9. `whitespace-padded seconds are honored` — `retry-after: " 2 "`; expect + `[2000]`. Proves the trim. +10. `hostile Retry-After vectors fall back to jitter` — one test looping over + `["0", "-5", "1e3", "0x10", ""]` with a fresh 429-then-ok queue and + `random: () => 0.5` per value; each expects exactly `[100]`. Pins the + strict parser against a `Number()`-swap regression. +11. `abort with a custom reason is not retried` — + `controller.abort(new Error("user cancel"))` before the call; fetch stub + rejects with `controller.signal.reason`; expect rejection with that exact + error (NOT wrapped in `XaiTokenRequestError`), 1 fetch call, zero sleeps. + Proves #4047 (reason-carrying abort). +12. `token request timeout is terminal` — fetch stub rejects with + `new DOMException("timed out", "TimeoutError")`, no caller abort; expect + rejection with `name: "TimeoutError"` (not wrapped), 1 fetch, zero sleeps. +13. `Bun-shaped timeout abort is terminal` — fetch stub rejects with + `new DOMException("The operation was aborted", "AbortError")` while the + caller signal is NOT aborted; expect rejection, 1 fetch, zero sleeps. +14. `caller aborted before a 429 backoff does not sleep` — abort inside the + fetch stub before returning the 429; expect rejection with the 429 + `XaiTokenRequestError` and zero sleeps. Proves the pre-sleep guard. +15. `caller abort during a Retry-After wait rejects promptly` — 429 with + `retry-after: 60`; injected `sleep` records its argument then returns a + never-resolving promise; the test body waits until the `60000` argument is + recorded, THEN calls `controller.abort(new Error("cancel during wait"))` + (never synchronously inside the injected sleep — `Promise.race` evaluates + `sleep(ms)` before the abort listener is attached); expect rejection with + that exact error, 1 fetch call, and the recorded sleep argument `60000`. + Proves the in-wait abort race (round-2 High fold). + +Existing-test compatibility (traced line by line by the round-2 auditor): +`network retry succeeds` still sleeps `[100]`; `429 and 5xx retry at most +three attempts` still sleeps `[100, 250]`; `third transient failure is +final` and `permanent 4xx` untouched; `caller abort is not retried` still +rejects with the `AbortError` DOMException via the `signal?.aborted` guard. + +## Verifier + +Remote: on the pull request, `ci.yml` runs the Linux `test` job, the macOS +`platform-macos` job, and the `gates` job (`tsc --noEmit`); the +`changes` filter covers `src/**` and `tests/**`, so +`tests/providers/xai/xai-oauth-retry.test.ts` executes in the Linux batches and +macOS shards. The Windows job (`platform-windows`) and `macos-control` are +NOT ON PR — they run only on `workflow_dispatch` with `lane=all`; the +cumulative final-head `lane=all` dispatch before merge is owned by the managing +task. The PR records the exact-head run URLs. Local: NOT RUN (`bun test`, +`bun run typecheck`) — user restriction; compile risk is covered by the +`gates` typecheck job, and the diff stays inside one already-typed function +cluster. + +## Audit record + +- Round 1 (four read-only xai/grok-4.6 issue verifiers): #4045 CONFIRMED; + #4046 PARTIALLY (sketch parser wrong — folded: strict donor parser); + #4047 PARTIALLY (Bun timeout surfaces as `AbortError`, per-attempt fresh + timer — folded: two-name guard, Bun-shaped test); #4048 CONFIRMED (phase 2). +- Round 2 (independent plan auditor): GO-WITH-FIXES (blockers=1) — HTTP-date + test was vacuous (folded: pinned random, assertion above the jitter cap); + CI map overstated Windows (folded); hostile parser vectors untested (folded); + donor-fidelity regex and truncated catch hunk (both folded). +- Round 2 (bounded retry-algorithm reviewer, via managing task): FAIL, one real + High — in-wait abort: `Bun.sleep` is not signal-aware, so an abort during a + honored 60 s wait could cancel up to ~120 s late. Folded: `sleepAbortable` + race with listener cleanup around the (possibly injected) sleep primitive, + pre-sleep and in-wait coverage, test 15. Managing-task invariant folded: a + server delay beyond the local budget must be terminal, never a silent early + retry — the retry-budget rule replaces the issue sketch's clamp. +- Round 3 (PR #4087 review bots on the published diff): Codex P1 — the 020 + phase-2 doc restated an unreleased endpoint-validation weakness and its + remediation in tracked devlog; folded by stripping 020 to a minimal stub with + all assessment/plan detail in gitignored scratch only. CodeRabbit Major — + RFC 9110 §5.6.7 requires recipients to accept all three HTTP-date formats; + folded by copying the donor parser at full fidelity (IMF-fixdate + RFC 850 + 50-year rule + asctime) and adding tests 6b/6c. diff --git a/devlog/_plan/260909_xai_oauth_retry_hardening/020_phase2_endpoint_validation.md b/devlog/_plan/260909_xai_oauth_retry_hardening/020_phase2_endpoint_validation.md new file mode 100644 index 0000000000..15e222db81 --- /dev/null +++ b/devlog/_plan/260909_xai_oauth_retry_hardening/020_phase2_endpoint_validation.md @@ -0,0 +1,19 @@ +# Phase 2 (wp3): xAI OAuth endpoint validation hardening + +Closes #4048 (security hardening). Separate PR layered on the phase-1 head +because it edits the same file. + +Per the repository security-notes policy (root `AGENTS.md`), everything about +this phase beyond the fact that it exists — the assessment, the patch plan, and +the test plan — is held in scratch space (`.tmp/260909_xai_endpoint_security/`, +gitignored) until the fix's own diff is public. A public issue describing a +weakness is not by itself license to restate the weakness, its blast radius, and +the remediation in a tracked document before the fix ships; the published +outcome (the merged diff and its release note) is what enters the record. + +## Phase entry condition + +Phase 2 starts only after phase 1's PR is published, and rebases onto the +published phase-1 head (manual chain: PR2 base = PR1 head branch). Its +pre-written scratch plan is re-verified against the rebased code before +implementation (LOOP-CONTINUITY-01). diff --git a/src/oauth/xai.ts b/src/oauth/xai.ts index f876b18b38..9af6b25a96 100644 --- a/src/oauth/xai.ts +++ b/src/oauth/xai.ts @@ -1,4 +1,5 @@ /** xAI OAuth flow (Grok account login). Ported from jawcode oauth/xai.ts. */ +import { abortError, sleepWithAbort } from "../lib/upstream-retry"; import { OAuthCallbackFlow, type OAuthCallbackFlowOptions } from "./callback-server"; import { generatePKCE } from "./pkce"; import type { LocalTokenImportMode, OAuthController, OAuthCredentials } from "./types"; @@ -11,6 +12,8 @@ const XAI_OAUTH_CALLBACK_PORT = 56121; const XAI_OAUTH_CALLBACK_PATH = "/callback"; const XAI_OAUTH_REFRESH_SKEW_MS = 2 * 60 * 1000; const TOKEN_REQUEST_TIMEOUT_MS = 30_000; +const RETRY_AFTER_MAX_DELAY_MS = 60_000; +const JITTER_DELAY_CAP_MS = 2_000; export const XAI_LOCAL_CLI_DETACH_WARNING = "[oauth:xai] Grok CLI credential was stale; refreshed into OpenCodex ownership. Grok CLI may require login again."; @@ -94,15 +97,135 @@ function getTokenIdentity(accessToken: string, idToken: string | undefined): { a export class XaiTokenRequestError extends Error { constructor(public readonly status?:number,public readonly oauthError?:string,message="xAI token request failed",options?:{cause?:unknown}){super(message,options);this.name="XaiTokenRequestError";} } export interface XaiTokenRetryDeps { sleep?:(ms:number)=>Promise; random?:()=>number } -function isAbortError(error:unknown):boolean{return error instanceof DOMException&&error.name==="AbortError";} -function retryDelay(attempt:number,retryAfter:string|null,random:()=>number):number{const base=attempt===1?100:250,j=Math.round(base*(.75+random()*.5)),seconds=retryAfter!==null&&/^\d+$/.test(retryAfter)?Number(retryAfter):0;return Math.min(2000,Math.max(j,seconds*1000));} -async function readTokenError(response:Response):Promise{let oauthError:string|undefined,detail="";try{const body=await response.json() as {error?:unknown;error_description?:unknown};if(typeof body.error==="string")oauthError=body.error;if(typeof body.error_description==="string")detail=body.error_description;}catch{}const suffix=detail?`: ${detail}`:oauthError?`: ${oauthError}`:"";return new XaiTokenRequestError(response.status,oauthError,`xAI token request failed: ${response.status}${suffix}`);} +const IMF_FIXDATE_RE = /^(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), (\d{2}) (Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) (\d{4}) (\d{2}):(\d{2}):(\d{2}) GMT$/i; +const RFC850_DATE_RE = /^(?:Monday|Tuesday|Wednesday|Thursday|Friday|Saturday|Sunday), (\d{2})-(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)-(\d{2}) (\d{2}):(\d{2}):(\d{2}) GMT$/i; +const ASCTIME_DATE_RE = /^(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun) (Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec) ( \d|\d{2}) (\d{2}):(\d{2}):(\d{2}) (\d{4})$/i; +const HTTP_MONTH_INDEX: Record = { + jan: 0, feb: 1, mar: 2, apr: 3, may: 4, jun: 5, + jul: 6, aug: 7, sep: 8, oct: 9, nov: 10, dec: 11, +}; + +function parseUtcDateParts( + year: number, + monthName: string, + day: number, + hour: number, + minute: number, + second: number, +): number | undefined { + const month = HTTP_MONTH_INDEX[monthName.toLowerCase()]; + if (month === undefined) return undefined; + const timestamp = Date.UTC(year, month, day, hour, minute, second); + const parsed = new Date(timestamp); + return parsed.getUTCFullYear() === year + && parsed.getUTCMonth() === month + && parsed.getUTCDate() === day + && parsed.getUTCHours() === hour + && parsed.getUTCMinutes() === minute + && parsed.getUTCSeconds() === second + ? timestamp + : undefined; +} + +function parseHttpDateMs(value: string, now: number): number | undefined { + const match = IMF_FIXDATE_RE.exec(value); + if (match) { + return parseUtcDateParts( + Number(match[3]), match[2]!, Number(match[1]), + Number(match[4]), Number(match[5]), Number(match[6]), + ); + } + const rfc850 = RFC850_DATE_RE.exec(value); + if (rfc850) { + // Two-digit years more than 50 years in the future are in the past (RFC 9110). + const currentYear = new Date(now).getUTCFullYear(); + let year = Math.floor(currentYear / 100) * 100 + Number(rfc850[3]); + const candidateTimeOfYear = Date.UTC( + 2000, HTTP_MONTH_INDEX[rfc850[2]!.toLowerCase()]!, Number(rfc850[1]), + Number(rfc850[4]), Number(rfc850[5]), Number(rfc850[6]), + ); + const current = new Date(now); + const currentTimeOfYear = Date.UTC( + 2000, current.getUTCMonth(), current.getUTCDate(), + current.getUTCHours(), current.getUTCMinutes(), current.getUTCSeconds(), + current.getUTCMilliseconds(), + ); + const yearDelta = year - currentYear; + if (yearDelta < -50 || (yearDelta === -50 && candidateTimeOfYear < currentTimeOfYear)) { + year += 100; + } else if (yearDelta > 50 || (yearDelta === 50 && candidateTimeOfYear > currentTimeOfYear)) { + year -= 100; + } + return parseUtcDateParts( + year, rfc850[2]!, Number(rfc850[1]), + Number(rfc850[4]), Number(rfc850[5]), Number(rfc850[6]), + ); + } + const asctime = ASCTIME_DATE_RE.exec(value); + if (!asctime) return undefined; + return parseUtcDateParts( + Number(asctime[6]), asctime[1]!, Number(asctime[2]), + Number(asctime[3]), Number(asctime[4]), Number(asctime[5]), + ); +} + +function parseRetryAfterMs(retryAfter: string | null): number | undefined { + const text = retryAfter?.trim(); + if (!text) return undefined; + if (/^\d+(?:\.\d+)?$/.test(text)) { + const ms = Math.ceil(Number(text) * 1000); + return ms > 0 ? ms : undefined; + } + const now = Date.now(); + const timestamp = parseHttpDateMs(text, now); + if (timestamp === undefined) return undefined; + const delay = timestamp - now; + return delay > 0 ? delay : undefined; +} + +function jitterDelay(attempt: number, random: () => number): number { + const base = attempt === 1 ? 100 : 250; + return Math.min(JITTER_DELAY_CAP_MS, Math.round(base * (0.75 + random() * 0.5))); +} + +/** + * Delay before the next attempt, or undefined when the server asked for a wait + * beyond the retry budget — retrying earlier than Retry-After would hammer the + * token endpoint, so the caller fails the request instead of clamping. + */ +function retryDelay(attempt: number, retryAfter: string | null, random: () => number): number | undefined { + const serverMs = parseRetryAfterMs(retryAfter); + if (serverMs === undefined) return jitterDelay(attempt, random); + return serverMs <= RETRY_AFTER_MAX_DELAY_MS ? serverMs : undefined; +} + +async function sleepAbortable( + ms: number, + sleep: (ms: number) => Promise, + signal: AbortSignal | undefined, +): Promise { + if (!signal) return sleep(ms); + if (signal.aborted) throw abortError(signal); + let onAbort!: () => void; + try { + await Promise.race([ + sleep(ms), + new Promise((_, reject) => { + onAbort = () => reject(abortError(signal)); + signal.addEventListener("abort", onAbort, { once: true }); + }), + ]); + } finally { + signal.removeEventListener("abort", onAbort); + } +} +async function readTokenError(response:Response):Promise{let oauthError:string|undefined,detail="";try{const body=await response.json() as {error?:unknown;error_description?:unknown};if(typeof body.error==="string")oauthError=body.error;if(typeof body.error_description==="string")detail=body.error_description;}catch{/* non-JSON error body: fall through to the generic message */}const suffix=detail?`: ${detail}`:oauthError?`: ${oauthError}`:"";return new XaiTokenRequestError(response.status,oauthError,`xAI token request failed: ${response.status}${suffix}`);} export async function postXaiToken( tokenEndpoint: string, body: Record, signal?: AbortSignal, deps:XaiTokenRetryDeps={}, ): Promise { - const sleep=deps.sleep??(ms=>Bun.sleep(ms)),random=deps.random??Math.random;let last:unknown; + const sleep=deps.sleep??((ms:number)=>sleepWithAbort(ms,signal)),random=deps.random??Math.random;let last:unknown; for(let attempt=1;attempt<=3;attempt++){let response:Response;try{response=await fetch(tokenEndpoint, { method: "POST", headers: { @@ -111,7 +234,15 @@ export async function postXaiToken( }, body: new URLSearchParams(body).toString(), signal: requestSignal(signal), - });}catch(error){if(isAbortError(error)&&signal?.aborted)throw error;last=error;if(attempt===3)throw new XaiTokenRequestError(undefined,undefined,"xAI token request failed: network error",{cause:error});await sleep(retryDelay(attempt,null,random));continue;}if(response.ok)return await response.json() as XaiTokenPayload;const error=await readTokenError(response);last=error;if(!(response.status===429||response.status>=500)||attempt===3)throw error;await sleep(retryDelay(attempt,response.headers.get("retry-after"),random));}throw last; + });}catch(error){ + if(signal?.aborted)throw error; + const name=(error as {name?:string}|undefined)?.name; + if(name==="AbortError"||name==="TimeoutError")throw error; + last=error; + if(attempt===3)throw new XaiTokenRequestError(undefined,undefined,"xAI token request failed: network error",{cause:error}); + await sleepAbortable(jitterDelay(attempt,random),sleep,signal); + continue; + }if(response.ok)return await response.json() as XaiTokenPayload;const error=await readTokenError(response);last=error;if(!(response.status===429||response.status>=500)||attempt===3)throw error;if(signal?.aborted)throw error;const delay=retryDelay(attempt,response.headers.get("retry-after"),random);if(delay===undefined)throw error;await sleepAbortable(delay,sleep,signal);}throw last; } function credentialsFromTokenPayload(payload: XaiTokenPayload, refreshFallback = ""): OAuthCredentials { diff --git a/tests/providers/xai/xai-oauth-retry.test.ts b/tests/providers/xai/xai-oauth-retry.test.ts index 542b594a3c..fd912d22f9 100644 --- a/tests/providers/xai/xai-oauth-retry.test.ts +++ b/tests/providers/xai/xai-oauth-retry.test.ts @@ -10,3 +10,31 @@ describe("xAI retry",()=>{ test("permanent 4xx is not retried or leaked",async()=>{const calls=queue([new Response(JSON.stringify({error:"invalid_grant"}),{status:400})]);await expect(postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async()=>{}})).rejects.toBeInstanceOf(XaiTokenRequestError);expect(calls()).toBe(1);}); test("caller abort is not retried",async()=>{const c=new AbortController();c.abort();let calls=0;globalThis.fetch=(async()=>{calls++;throw new DOMException("aborted","AbortError")}) as typeof fetch;await expect(postXaiToken("https://auth.x.ai/token",body,c.signal,{sleep:async()=>{}})).rejects.toMatchObject({name:"AbortError"});expect(calls).toBe(1);}); }); + +describe("xAI Retry-After handling",()=>{ + const ra=(value:string)=>new Response("",{status:429,headers:{"retry-after":value}}); + const WEEKDAYS=["Sunday","Monday","Tuesday","Wednesday","Thursday","Friday","Saturday"],MONTHS=["Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"]; + const hm=d=>`${String(d.getUTCHours()).padStart(2,"0")}:${String(d.getUTCMinutes()).padStart(2,"0")}:${String(d.getUTCSeconds()).padStart(2,"0")}`; + const rfc850=(d:Date)=>`${WEEKDAYS[d.getUTCDay()]}, ${String(d.getUTCDate()).padStart(2,"0")}-${MONTHS[d.getUTCMonth()]}-${String(d.getUTCFullYear()%100).padStart(2,"0")} ${hm(d)} GMT`; + const asctime=(d:Date)=>`${WEEKDAYS[d.getUTCDay()]!.slice(0,3)} ${MONTHS[d.getUTCMonth()]} ${String(d.getUTCDate()).padStart(2," ")} ${hm(d)} ${d.getUTCFullYear()}`; + test("429 honors Retry-After seconds beyond the jitter cap",async()=>{const calls=queue([ra("60"),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(calls()).toBe(2);expect(d).toEqual([60000]);}); + test("Retry-After above the 60s budget is terminal, never retried early",async()=>{const calls=queue([ra("3600")]),d:number[]=[];await expect(postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5})).rejects.toMatchObject({status:429});expect(calls()).toBe(1);expect(d).toEqual([]);}); + test("Retry-After one second above the budget is terminal",async()=>{const calls=queue([ra("61")]),d:number[]=[];await expect(postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5})).rejects.toMatchObject({status:429});expect(calls()).toBe(1);expect(d).toEqual([]);}); + test("Retry-After below the old 2s cap is honored exactly",async()=>{const calls=queue([ra("1"),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(d).toEqual([1000]);}); + test("fractional Retry-After is honored",async()=>{const calls=queue([ra("1.5"),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(d).toEqual([1500]);}); + test("HTTP-date Retry-After is honored",async()=>{const calls=queue([ra(new Date(Date.now()+30_000).toUTCString()),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(calls()).toBe(2);expect(d.length).toBe(1);expect(d[0]!).toBeGreaterThan(2000);expect(d[0]!).toBeLessThanOrEqual(30000);}); + test("RFC 850 HTTP-date Retry-After is honored",async()=>{const calls=queue([ra(rfc850(new Date(Date.now()+30_000))),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(calls()).toBe(2);expect(d.length).toBe(1);expect(d[0]!).toBeGreaterThan(2000);expect(d[0]!).toBeLessThanOrEqual(30000);}); + test("asctime HTTP-date Retry-After is honored",async()=>{const calls=queue([ra(asctime(new Date(Date.now()+30_000))),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(calls()).toBe(2);expect(d.length).toBe(1);expect(d[0]!).toBeGreaterThan(2000);expect(d[0]!).toBeLessThanOrEqual(30000);}); + test("unparseable Retry-After falls back to jitter",async()=>{const calls=queue([ra("soon"),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(d).toEqual([100]);}); + test("past HTTP-date falls back to jitter",async()=>{const calls=queue([ra("Sun, 06 Nov 1994 08:49:37 GMT"),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(d).toEqual([100]);}); + test("whitespace-padded seconds are honored",async()=>{const calls=queue([ra(" 2 "),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(d).toEqual([2000]);}); + test("hostile Retry-After vectors fall back to jitter",async()=>{for(const v of ["0","-5","1e3","0x10",""]){const calls=queue([ra(v),ok()]),d:number[]=[];await postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)},random:()=>.5});expect(d).toEqual([100]);expect(calls()).toBe(2);}}); +}); + +describe("xAI abort handling",()=>{ + test("abort with a custom reason is not retried",async()=>{const c=new AbortController();const reason=new Error("user cancel");let calls=0;globalThis.fetch=(async()=>{calls++;c.abort(reason);throw c.signal.reason;}) as typeof fetch;const d:number[]=[];await expect(postXaiToken("https://auth.x.ai/token",body,c.signal,{sleep:async x=>{d.push(x)}})).rejects.toBe(reason);expect(calls).toBe(1);expect(d).toEqual([]);}); + test("token request timeout is terminal",async()=>{let calls=0;globalThis.fetch=(async()=>{calls++;throw new DOMException("timed out","TimeoutError");}) as typeof fetch;const d:number[]=[];await expect(postXaiToken("https://auth.x.ai/token",body,undefined,{sleep:async x=>{d.push(x)}})).rejects.toMatchObject({name:"TimeoutError"});expect(calls).toBe(1);expect(d).toEqual([]);}); + test("Bun-shaped timeout abort is terminal",async()=>{const c=new AbortController();let calls=0;globalThis.fetch=(async()=>{calls++;throw new DOMException("The operation was aborted","AbortError");}) as typeof fetch;const d:number[]=[];await expect(postXaiToken("https://auth.x.ai/token",body,c.signal,{sleep:async x=>{d.push(x)}})).rejects.toMatchObject({name:"AbortError"});expect(calls).toBe(1);expect(d).toEqual([]);}); + test("caller aborted before a 429 backoff does not sleep",async()=>{const c=new AbortController();let calls=0;globalThis.fetch=(async()=>{calls++;c.abort();return new Response("",{status:429,headers:{"retry-after":"60"}});}) as typeof fetch;const d:number[]=[];await expect(postXaiToken("https://auth.x.ai/token",body,c.signal,{sleep:async x=>{d.push(x)}})).rejects.toMatchObject({status:429});expect(calls).toBe(1);expect(d).toEqual([]);}); + test("caller abort during a Retry-After wait rejects promptly",async()=>{const c=new AbortController();const reason=new Error("cancel during wait");globalThis.fetch=(async()=>new Response("",{status:429,headers:{"retry-after":"60"}})) as typeof fetch;const d:number[]=[];const pending=postXaiToken("https://auth.x.ai/token",body,c.signal,{sleep:async x=>{d.push(x);await new Promise(()=>{});}});while(!d.length)await Bun.sleep(1);c.abort(reason);await expect(pending).rejects.toBe(reason);expect(d).toEqual([60000]);}); +});