diff --git a/devlog/_plan/260904_bug_stack_train/000_research.md b/devlog/_plan/260904_bug_stack_train/000_research.md new file mode 100644 index 00000000000..ac4c1cba78b --- /dev/null +++ b/devlog/_plan/260904_bug_stack_train/000_research.md @@ -0,0 +1,79 @@ +# 000 — Live manifest and disposition research + +Snapshot taken 2026-09-04, base `origin/dev` = `b5777aa2d642`. +Worktree: `/Users/jun/.codex/worktrees/9d5b/opencodex`. + +All findings below were produced by four parallel read-only research lanes and +re-checked against the current tree. Every disposition names its evidence. + +## Open bug-labelled PRs (10) + +| PR | Author | Verdict | Basis | +|----|--------|---------|-------| +| #3335 | x3M3x | LAND_AS_IS | GUI hardcodes 2 of 5 strategies at `gui/src/components/combo-workspace-controls.tsx:24-50`; canonical set already has 5 at `gui/src/combo-workspace-data.ts:11-30`. Test is RED without the fix. | +| #3333 | blackjune67 | LAND_AS_IS | Models panels are persistent and toggle `hidden` (`gui/src/pages/Models.tsx:2228-2312`); scoping to the visible panel id stops width leakage. Test asserts selectors absent on dev. | +| #3322 | luvs01 | LAND_AS_IS | Head already implements the exact requested message at `src/cli/observe.ts:75-77`. The `CHANGES_REQUESTED` review is stale against the corrected head. | +| #3357 | huaiqing-afk | LAND_AS_IS (draft) | One global previous-text slot at `src/adapters/cursor/protobuf-request.ts:303-381` lets every tool result reset narration detection. PR tracks roles independently. Strong RED regression. | +| #3325 | luvs01 | BLOCKED_ON_POLICY | Code correct, but `.github/workflows/` is a restricted surface (`.github/scripts/pr-sponsored-surface.cjs:24-27`); hygiene fails `unsponsored_surface` without the `maintainer-sponsored` label. The second "failure" is a cancelled `enforce-target` run, not a real failure. | +| #3364 | lidge-jun | LAND_WITH_FIX | Exact-head CI green. Missing a direct `parseResponse()` non-stream regression even though production parse calls the same extractor (`src/adapters/openai-responses.ts:2450-2481`). | +| #3361 | lidge-jun | LAND_AS_IS | Exact-head CI green; marker/journal ownership preserved per key; `startServer` stays synchronous. Touches unauthenticated loopback admission, so it needs explicit maintainer security sign-off. | +| #3332 | full999 | LAND_WITH_FIX | Writes an OUTPUT limit into an INPUT field: `ModelMetadata.maxTokens` is output (`src/generated/model-metadata.ts:4-12`) but lands in `maxInputTokens`. Would shrink Claude 1M input models to 64K/128K. | +| #3348 | RHODIZSECURITY | DEFER | 2,248 lines / 34 files across failover, credentials, persistence, shutdown, and the core response path. Confirmed blocker: generic HTTP 410/413 become retryable hops, so an oversized or invalid request is replayed to the next provider. | +| #3312 | RHODIZSECURITY | DEFER (superseded) | Functionally the same work as #3348 with the same 410/413 blocker; currently CONFLICTING/DIRTY. Not an ancestry successor, but #3348 supersedes it. | + +## Open bug-labelled issues (6) + +None are safely fixable from the evidence currently attached. Detail: + +- **#3352** (GPT-5.6 401) — NEEDS_REPORTER_EVIDENCE. Mechanism is established end to end: + gating at `src/codex/catalog/native-models.ts:5`, roster fetch at + `src/codex/model-entitlements.ts:185`, unconfirmed-evidence fallback at `:548`, + granted-only projection at `:958`/`:1024`, and the exact 401 at + `src/codex/auth-context.ts:435`. The reported `0.142.2` floor theory is already + ruled out — the code enforces `0.144.0` at `:75`. Letting `unknown` through would + be a security-policy change, not a bug fix. +- **#3320** (Windows non-ASCII scheduler) — NEEDS_REPORTER_EVIDENCE. Production XML + writes a locale-independent SID (`src/service.ts:1841,1912`); exact `` + matching is deliberate (`:2117`) because folding two non-ASCII identities to `???` + could adopt another account's task. Needs redacted live XML before any patch. +- **#3279** (GUI 401) — NEEDS_REPORTER_EVIDENCE. Each page load mints a session from + its own Host-derived origin (`src/server/gui-session.ts:166`); exact origin checks + are the admission boundary (`:417`); expiry is deterministic at 5 minutes (`:62`). + Canonicalizing localhost/IPv4/IPv6 would weaken auth without proving cause. +- **#3255** (capability vs speed) — PRODUCT_DECISION. The two dimensions are already + independent (`src/reasoning-effort.ts:5` vs `src/codex/catalog/effort.ts:160`), and + there is no Ultra-fast wire tier to pass through. +- **#3245** (stream disconnect) — NEEDS_REPORTER_EVIDENCE. 426 is intentional + (`src/server/index.ts:1107`) and the 426-then-POST path is already covered + (`tests/server-auth.test.ts:1384`). The reporter saw no subsequent POST, which puts + the failure before the Responses bridge. +- **#1527** (Cursor large context) — NEEDS_REPORTER_EVIDENCE. Every known defect in + this path is already fixed; a matched current-dev trace is required. + +## Issue #3366 — deviceauth (the implementation target) + +Key correction to the issue's premise: `chatgpt` is deliberately excluded from the +generic OAuth surface (`src/oauth/index.ts:284-297`, `tests/oauth-public-surface.test.ts:77-111`) +and `openai|codex|chatgpt` route through the separate Codex-auth API. Returning +`deviceCode` from `src/oauth/` alone therefore does NOT light up the existing UI — +the Codex-auth layer discards it today at `src/codex/auth-api.ts:2199-2209`. + +Upstream wire flow, confirmed against `codex-rs/login/src/device_code_auth.rs`: +15-minute poll window, only 403/404 mean pending, server-issued `code_verifier`, +and `redirect_uri=https://auth.openai.com/deviceauth/callback`. + +Non-fabrication note: the issue claims a `codex_cli_rs` User-Agent is required. +Upstream actually builds a raw auth client with no Codex default headers +(`device_code_auth.rs:165-171`), and its real UA is dynamic. We do not hard-code +client impersonation; we send no custom UA and let the platform default stand. + +## Stack plan + +Dependency-ordered, bottom-up (DEV-STACK-01): + +1. `codex/deviceauth-core` — the grant itself in `src/oauth/` (010) +2. `codex/deviceauth-surface` — Codex-auth API + CLI + docs (020) +3. `codex/bug-carry` — carried contributor fixes with attribution (030) + +Deferred out of the stack with recorded reasons: #3348, #3312, #3325, and all six +bug issues. Documented in 040. diff --git a/devlog/_plan/260904_bug_stack_train/010_wp2_deviceauth_core.md b/devlog/_plan/260904_bug_stack_train/010_wp2_deviceauth_core.md new file mode 100644 index 00000000000..eb3de69d315 --- /dev/null +++ b/devlog/_plan/260904_bug_stack_train/010_wp2_deviceauth_core.md @@ -0,0 +1,72 @@ +# 010 — wp2: deviceauth grant core (stack layer 1) + +Branch: `codex/deviceauth-core`, based on `origin/dev` `b5777aa2d642`. +Thesis: implement the OpenAI deviceauth grant as a self-contained module and let +`loginChatGPT` select it. Nothing outside `src/oauth/` changes in this layer. + +## Files + +- ADD `src/oauth/chatgpt-device.ts` — the grant. +- MODIFY `src/oauth/chatgpt.ts` — export `credsFromToken` for reuse; add the + `flow` option to `loginChatGPT`. +- MODIFY `src/oauth/index.ts` — thread `flow` through the `chatgpt` registry entry. +- ADD `tests/chatgpt-device-auth.test.ts`. +- MODIFY `tests/oauth-device-code-contract.test.ts` — extend the shared contract to chatgpt. + +## Wire protocol (from codex-rs device_code_auth.rs) + +1. `POST https://auth.openai.com/api/accounts/deviceauth/usercode` + JSON `{ client_id }` -> `{ device_auth_id, user_code, interval? }` +2. `POST https://auth.openai.com/api/accounts/deviceauth/token` + JSON `{ device_auth_id, user_code }`; 403/404 = pending; 200 = + `{ authorization_code, code_verifier }` +3. `POST https://auth.openai.com/oauth/token` form-encoded + `grant_type=authorization_code`, `client_id`, `code`, `code_verifier`, + `redirect_uri=https://auth.openai.com/deviceauth/callback` + +Poll window 15 minutes; default interval 5s; the interval field may arrive as a +string, so coerce numerically and floor at 1s. + +## Signatures + +```ts +export type ChatGPTLoginFlow = "browser" | "device"; +export async function loginChatGPTDevice(ctrl: OAuthController): Promise; +export async function loginChatGPT( + ctrl: OAuthController, + opts?: { forceLogin?: boolean; flow?: ChatGPTLoginFlow }, +): Promise; +``` + +`onAuth` publishes `{ url: "https://auth.openai.com/codex/device", deviceCode: user_code, +instructions }` — matching the kimi/nous/copilot contract where `deviceCode` carries the +HUMAN code, never the opaque polling handle. + +## Credential boundary + +- Never log `device_auth_id`, `authorization_code`, `code_verifier`, or any token. +- Do NOT reuse `safeErrorDescription` from the callback flow: it reflects upstream + body text. Device errors carry status only. +- Bound the success payload; reject non-string `authorization_code`/`code_verifier`. + +## Tests (red-then-green) + +`tests/chatgpt-device-auth.test.ts`, stubbing `globalThis.fetch` by URL in the +established style of `tests/oauth-device-code-contract.test.ts:16-63`: + +1. requests a user code and surfaces the fixed verification URL + human code +2. treats only 403/404 as pending and honors the returned interval +3. exchanges the server-issued `authorization_code`/`code_verifier` at the device callback URI +4. rejects a malformed success payload without reflecting the body +5. aborts promptly on signal +6. surfaces `accountId`/`email` from a realistic device-token `id_token`, because Codex + pool admission rejects a credential with no account id (`src/codex/auth-api.ts:2221`). + Wire success alone is not proof the credential is usable. + +Focused command: `bun test tests/chatgpt-device-auth.test.ts tests/oauth-device-code-contract.test.ts tests/chatgpt-oauth.test.ts` + +## Security review gate + +`src/oauth/` is a restricted authentication surface +(`.github/scripts/pr-sponsored-surface.cjs:24`); `MAINTAINERS.md:60` requires explicit +security review. The PR description states this; it does not merge as routine work. diff --git a/devlog/_plan/260904_bug_stack_train/020_wp3_deviceauth_surface.md b/devlog/_plan/260904_bug_stack_train/020_wp3_deviceauth_surface.md new file mode 100644 index 00000000000..ba954946cdd --- /dev/null +++ b/devlog/_plan/260904_bug_stack_train/020_wp3_deviceauth_surface.md @@ -0,0 +1,67 @@ +# 020 — wp3: deviceauth surface (stack layer 2) + +Branch: `codex/deviceauth-surface`, based on `codex/deviceauth-core`. +Thesis: make the grant reachable. Without this layer the core is unreachable from any +user-facing path, because `openai|codex|chatgpt` go through the Codex-auth API, which +drops `deviceCode` from the start DTO at `src/codex/auth-api.ts:2440` and opens the +authorization URL at `:2206` (conditional on `shouldOpenBrowserForLogin`, which already +honors an explicit/configured false at `src/oauth/open-browser-choice.ts:20` — the gap is +that a device flow is not itself a reason to skip the open). + +## Files + +- MODIFY `src/codex/auth-api.ts` — accept `device?: boolean` on login start, pass + `flow: "device"` into the chatgpt login, return `deviceCode` in the start DTO, and + suppress the server-side browser open when `deviceCode` is present (mirroring + `src/server/management/oauth-account-routes.ts:185`). +- MODIFY `src/cli/account-auth.ts` — add `--device`; include it in the login body; + print `Device code: ` in the Codex pre-poll block; preserve it under + `--no-wait --json`. +- MODIFY `src/cli/capabilities.ts` — declare the flag. +- MODIFY `gui/src/components/use-add-codex-account-oauth.ts` — keep `deviceCode` and + `instructions` on the start DTO (dropped today at `:148`) and request device mode. +- MODIFY `gui/src/components/add-codex-account-reducer.ts` — carry both fields in state. +- MODIFY `gui/src/components/add-codex-account-waiting-step.tsx` — pass them to + `LoginHint` (today it passes only `url` at `:38`). The shared renderer at + `gui/src/components/login-url-block.tsx:42-47,73-107` is already device-capable, so no + new UI component is needed. +- REGENERATE `skills/ocx/references/01_management_surface.md` via `bun run skill:surface` + (gated by `tests/skill-ocx.test.ts`). +- MODIFY `docs-site/` provider/account docs (English source; do not let locales contradict). + +## Poll budget (audit blocker 2) + +The device grant lives 15 minutes, but both existing poll budgets stop at five: +Codex-auth polls 150 x 2s and then records an error (`src/codex/auth-api.ts:2214,2414`), +and the CLI independently stops at the same 150 x 2s (`src/cli/account-auth.ts:123`). +Shipping the grant without widening these would advertise a 15-minute window that +dies at minute five — exactly the headless case this feature exists for, where the +operator walks to another device to enter the code. + +Both budgets are raised for the device flow, and a test proves a login completing +after minute five still succeeds (fake timers; no real waiting). + +## Security review gate (audit finding 4) + +`src/oauth/`, `src/codex/auth-api.ts`, and `src/cli/account-auth.ts` are restricted +authentication surfaces (`.github/scripts/pr-sponsored-surface.cjs:24`) and require +explicit security review per `MAINTAINERS.md:60`. Both deviceauth PRs carry that +requirement in their description; neither is merged as routine. + +Explicitly NOT done: repurposing `--code` as device user-code input. The device user +code is entered at `auth.openai.com`, while `account code` submits callback +authorization material to a different endpoint (`src/codex/auth-api.ts:2457-2472`). +Conflating them would silently break the existing paste fallback. + +## Tests + +- `tests/codex-auth-api.test.ts`: device login returns `deviceCode` and does not open a URL. +- `tests/cli-account.test.ts`: `--device` prints URL + device code + flow id; `--no-wait --json` preserves it. +- `tests/codex-auth-api.test.ts`: a device login that completes after minute five still succeeds. +- `tests/cli-account.test.ts`: with fake timers, a normal (polling) `--device` login completes + after minute five. The `--no-wait` case bypasses polling and does not cover this. +- `gui/tests/add-codex-account-device.test.tsx`: the start request carries `device: true`, + and the waiting step renders the device code and verification URL. + +Focused: `bun test tests/codex-auth-api.test.ts tests/cli-account.test.ts tests/skill-ocx.test.ts` +plus the single focused GUI test file. No repository-wide suite. diff --git a/devlog/_plan/260904_bug_stack_train/030_wp4_bug_carry.md b/devlog/_plan/260904_bug_stack_train/030_wp4_bug_carry.md new file mode 100644 index 00000000000..3e4294bd48c --- /dev/null +++ b/devlog/_plan/260904_bug_stack_train/030_wp4_bug_carry.md @@ -0,0 +1,39 @@ +# 030 — wp4: carried contributor fixes (PARALLEL, not stacked) + +Corrected after plan audit. These were originally drafted as a third stack layer above +deviceauth. That was wrong: none of the four consumes deviceauth and none consumes +another, so stacking them would impose a false merge order. DEV-STACK-01 says +independent parts open as parallel PRs off trunk, and DEV-STACK-03 says one thesis per +layer — four unrelated theses in one layer violates both. + +Each fix therefore gets its own branch off `dev`, merged independently: +`codex/carry-3335`, `codex/carry-3333`, `codex/carry-3322`, `codex/carry-3357`. + +Four PRs were judged root-correct with RED-without-fix regressions. Each is carried as +its own independent PR with a `Co-authored-by` trailer in its commit, so the +contributor graph records the author (AGENTS.md; `CREDITS.md` exists because 27 +landings previously lost attribution). + +| Source PR | Author trailer | Scope | +|-----------|----------------|-------| +| #3335 | `Co-authored-by: x3M3x ` | GUI combo strategy selector: render all five | +| #3333 | `Co-authored-by: hajune ` | Models tab spacing + Combos layout stability | +| #3322 | `Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>` | `logs --follow` capability contract | +| #3357 | `Co-authored-by: huaiqing-afk ` | Cursor repeated-narration breaker | + +Carry method: fetch the PR head and re-apply its source/test hunks onto a fresh branch +off `dev`, one commit per source PR, each carrying its trailer. Do not pipe +`gh pr diff` straight into `git apply` for #3335 — GitHub emits binary PNG hunks +without full index data, so the whole-patch check fails on the two +`docs/pr-assets/*.png` files even though every source hunk applies cleanly. + +Focused verification, per branch — each branch runs only its own tests: + +| Branch | Command | +|--------|---------| +| `codex/carry-3335` | `cd gui && bun test tests/combo-strategy-selector.test.tsx` | +| `codex/carry-3333` | `cd gui && bun test tests/models-tab-layout.test.ts` | +| `codex/carry-3322` | `bun test tests/cli-usage-report.test.ts tests/cli-capabilities.test.ts` | +| `codex/carry-3357` | `bun test tests/cursor-repetition-breaker.test.ts` | + +No repository-wide suite (explicit user constraint). diff --git a/devlog/_plan/260904_bug_stack_train/040_deferrals.md b/devlog/_plan/260904_bug_stack_train/040_deferrals.md new file mode 100644 index 00000000000..d1e6da02b2e --- /dev/null +++ b/devlog/_plan/260904_bug_stack_train/040_deferrals.md @@ -0,0 +1,30 @@ +# 040 — Recorded deferrals + +Deferring is a disposition, not an omission. Each item below stays open with a +stated reason rather than being force-landed. + +## #3348 / #3312 — combos failover hardening + +Both carry the same confirmed correctness blocker: generic HTTP 410 and 413 are +classified as retryable hops (`src/combos/failover.ts:563-617` on both heads), so an +oversized or invalid request would be replayed to the next provider. Their own tests +encode the wrong expectation. #3348 functionally supersedes #3312 (30 shared files, +near-identical source diffs; #3312 is additionally CONFLICTING/DIRTY). + +At 2,248 lines across 34 files spanning failover, credential rotation, durable +cooldown persistence, shutdown, and the core response path, this is not reviewable +inside a mixed campaign. It needs its own split stack. + +## #3325 — dev bump guard fork filter + +The code is correct, but `.github/workflows/` is a restricted surface +(`.github/scripts/pr-sponsored-surface.cjs:24-27`) and the hygiene gate fails +`unsponsored_surface` without a maintainer sponsorship decision. That is a policy +action for a human, not a patch. Note the second red check is a cancelled +`enforce-target` run that `gh pr checks` renders as a failure. + +## All six bug issues + +See 000. Every one needs reporter evidence or a product decision. Three of them +(#3352, #3320, #3279) would require weakening an auth or identity boundary to +"fix" without a reproduction, which is the wrong trade. diff --git a/src/oauth/chatgpt-device.ts b/src/oauth/chatgpt-device.ts new file mode 100644 index 00000000000..74fbeeabd9c --- /dev/null +++ b/src/oauth/chatgpt-device.ts @@ -0,0 +1,187 @@ +import type { OAuthController, OAuthCredentials } from "./types"; +import { CHATGPT_CLIENT_ID, CHATGPT_TOKEN_URL, credsFromToken } from "./chatgpt"; + +/** + * OpenAI deviceauth (device-code) grant for the ChatGPT/Codex provider. + * + * The callback flow in `./chatgpt` needs a browser and a listener on + * localhost:1455. A hub running headless in a container or over SSH has + * neither, which left "copy the long redirect URL out of the browser error + * page" as the only way to add an account there (#3366). + * + * This is the same grant Codex CLI uses. Three steps, and the middle one is + * where it differs from RFC 8628: the poll returns an authorization code plus + * a SERVER-generated PKCE verifier, which is then spent at the ordinary token + * endpoint. We never generate the verifier ourselves here. + */ +const USERCODE_URL = "https://auth.openai.com/api/accounts/deviceauth/usercode"; +const DEVICE_TOKEN_URL = "https://auth.openai.com/api/accounts/deviceauth/token"; +const DEVICE_REDIRECT_URI = "https://auth.openai.com/deviceauth/callback"; + +/** Where the user types the short code. Fixed, and safe to show anywhere. */ +export const DEVICE_VERIFICATION_URL = "https://auth.openai.com/codex/device"; + +/** The grant's own lifetime. Polling past this only produces a worse error message. */ +const DEVICE_FLOW_TTL_MS = 15 * 60 * 1000; +const DEFAULT_POLL_INTERVAL_MS = 5_000; +const MIN_POLL_INTERVAL_MS = 1_000; +/** + * Above ~2^31 ms a timer overflows and fires immediately, which would turn a + * hostile or corrupt `interval` into a hot loop against an auth endpoint. The + * grant only lives 15 minutes, so anything longer is meaningless anyway. + */ +const MAX_POLL_INTERVAL_MS = DEVICE_FLOW_TTL_MS; + +/** + * Upstream sends `interval` as a number in some responses and a string in + * others. A string would make `setTimeout` treat it as 0 and turn the poll + * into a hot loop against an auth endpoint, so coerce and floor it. + */ +function normalizeIntervalMs(raw: unknown): number { + const seconds = typeof raw === "number" ? raw : typeof raw === "string" ? Number(raw) : NaN; + if (!Number.isFinite(seconds) || seconds <= 0) return DEFAULT_POLL_INTERVAL_MS; + const ms = Math.round(seconds * 1000); + return Math.min(MAX_POLL_INTERVAL_MS, Math.max(MIN_POLL_INTERVAL_MS, ms)); +} + +function nonEmptyString(value: unknown): string | undefined { + return typeof value === "string" && value.length > 0 ? value : undefined; +} + +async function sleep(ms: number, signal?: AbortSignal): Promise { + if (signal?.aborted) throw new Error("Login cancelled"); + await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + signal?.removeEventListener("abort", onAbort); + resolve(); + }, ms); + const onAbort = (): void => { + clearTimeout(timer); + reject(new Error("Login cancelled")); + }; + signal?.addEventListener("abort", onAbort, { once: true }); + }); +} + +/** + * Device-flow errors carry the HTTP status and nothing else. + * + * The callback flow's `safeErrorDescription` reflects the upstream body into + * the message, which is fine for an OAuth error envelope but not here: these + * endpoints can echo request material, and this message reaches CLI output, + * the GUI, and issue reports. + */ +function deviceError(stage: string, status: number): Error { + return new Error(`ChatGPT device authorization ${stage} failed: HTTP ${status}`); +} + +interface DeviceUserCode { + deviceAuthId: string; + userCode: string; + intervalMs: number; +} + +async function requestUserCode(signal?: AbortSignal): Promise { + const response = await fetch(USERCODE_URL, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ client_id: CHATGPT_CLIENT_ID }), + signal, + }); + if (!response.ok) throw deviceError("request", response.status); + const payload = (await response.json()) as Record; + const deviceAuthId = nonEmptyString(payload.device_auth_id); + // Upstream accepts both spellings, so a response using the alias must not be + // rejected as malformed. + const userCode = nonEmptyString(payload.user_code) ?? nonEmptyString(payload.usercode); + if (!deviceAuthId || !userCode) { + throw new Error("ChatGPT device authorization response missing required fields"); + } + return { deviceAuthId, userCode, intervalMs: normalizeIntervalMs(payload.interval) }; +} + +interface DeviceGrant { + authorizationCode: string; + codeVerifier: string; +} + +/** + * Poll until the user finishes at the verification page. + * + * Pending is signalled by 403/404 rather than an `authorization_pending` body, + * so status is the whole protocol here: any other non-2xx is terminal, and + * treating it as pending would keep hammering a permanently failing endpoint. + */ +async function pollForGrant( + device: DeviceUserCode, + signal?: AbortSignal, +): Promise { + const deadline = Date.now() + DEVICE_FLOW_TTL_MS; + while (Date.now() < deadline) { + if (signal?.aborted) throw new Error("Login cancelled"); + const response = await fetch(DEVICE_TOKEN_URL, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ device_auth_id: device.deviceAuthId, user_code: device.userCode }), + signal, + }); + if (response.status === 403 || response.status === 404) { + // Cap the wait at the time actually left. Sleeping a full interval past + // the deadline is how a 15-minute grant turns into a 20-minute wait. + const remaining = deadline - Date.now(); + if (remaining <= 0) break; + await sleep(Math.min(device.intervalMs, remaining), signal); + continue; + } + if (!response.ok) throw deviceError("poll", response.status); + // The deadline is checked again here, not only at the top of the loop: a + // single poll can itself outlive the grant, and accepting a code that + // expired mid-flight just moves the failure to the token exchange. + if (Date.now() >= deadline) break; + const payload = (await response.json()) as Record; + const authorizationCode = nonEmptyString(payload.authorization_code); + const codeVerifier = nonEmptyString(payload.code_verifier); + if (!authorizationCode || !codeVerifier) { + throw new Error("ChatGPT device authorization response missing required fields"); + } + return { authorizationCode, codeVerifier }; + } + throw new Error("ChatGPT device authorization expired"); +} + +async function exchangeGrant(grant: DeviceGrant, signal?: AbortSignal): Promise { + const response = await fetch(CHATGPT_TOKEN_URL, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + grant_type: "authorization_code", + client_id: CHATGPT_CLIENT_ID, + code: grant.authorizationCode, + code_verifier: grant.codeVerifier, + redirect_uri: DEVICE_REDIRECT_URI, + }).toString(), + signal, + }); + if (!response.ok) throw deviceError("token exchange", response.status); + return credsFromToken((await response.json()) as Record); +} + +/** + * Run the device flow to completion. + * + * `deviceCode` in the `onAuth` payload is the HUMAN code, matching kimi, nous, + * and github-copilot. The opaque `device_auth_id` never leaves this module: + * every device-code surface renders `deviceCode` verbatim, and the management + * login route also uses its presence to decide a flow must not be handed to a + * local browser spawn. + */ +export async function loginChatGPTDevice(ctrl: OAuthController): Promise { + const device = await requestUserCode(ctrl.signal); + ctrl.onAuth?.({ + url: DEVICE_VERIFICATION_URL, + instructions: `Enter code: ${device.userCode}`, + deviceCode: device.userCode, + }); + const grant = await pollForGrant(device, ctrl.signal); + return exchangeGrant(grant, ctrl.signal); +} diff --git a/src/oauth/chatgpt.ts b/src/oauth/chatgpt.ts index 5dd01497db3..bb4d1c84978 100644 --- a/src/oauth/chatgpt.ts +++ b/src/oauth/chatgpt.ts @@ -5,6 +5,10 @@ import { generatePKCE } from "./pkce"; const CLIENT_ID = "app_EMoamEEZ73f0CkXaXp7hrann"; const AUTH_URL = "https://auth.openai.com/oauth/authorize"; const TOKEN_URL = "https://auth.openai.com/oauth/token"; + +/** Shared with the deviceauth grant in `./chatgpt-device`: same public PKCE client. */ +export const CHATGPT_CLIENT_ID = CLIENT_ID; +export const CHATGPT_TOKEN_URL = TOKEN_URL; const SCOPE = "openid profile email offline_access api.connectors.read api.connectors.invoke"; const CALLBACK_PORT = 1455; const CALLBACK_PATH = "/auth/callback"; @@ -46,9 +50,17 @@ export function extractEmail(idToken?: string, accessToken?: string): string | u return undefined; } -function credsFromToken(data: Record): OAuthCredentials { +export function credsFromToken(data: Record): OAuthCredentials { const idToken = typeof data.id_token === "string" ? data.id_token : undefined; - const accessToken = data.access_token as string; + // This parses a response from an external boundary, so the access token is + // validated rather than cast. A 200 carrying no access_token would otherwise + // resolve a login as successful with an undefined credential, which then gets + // silently declined at persistence — a success message and no account. + const accessToken = typeof data.access_token === "string" && data.access_token.length > 0 + ? data.access_token + : undefined; + if (!accessToken) throw new Error("ChatGPT token response missing access token"); + const refreshToken = typeof data.refresh_token === "string" ? data.refresh_token : ""; // ?? only guards null/undefined; NaN or a string expires_in would otherwise // produce a NaN expiry that never compares as expired, and a negative duration // would stamp an already-past expiry — both block refresh semantics. @@ -62,7 +74,7 @@ function credsFromToken(data: Record): OAuthCredentials { const expires = Number.isFinite(computedExpires) ? computedExpires : Date.now() + 3600 * 1000; return { access: accessToken, - refresh: (data.refresh_token as string) ?? "", + refresh: refreshToken, expires, accountId: extractAccountId(idToken, accessToken), email: extractEmail(idToken, accessToken), @@ -135,7 +147,22 @@ function safeErrorDescription(resp: Response): Promise { }); } -export async function loginChatGPT(ctrl: OAuthController, opts?: { forceLogin?: boolean }): Promise { +/** + * How the user proves identity. `browser` runs the localhost:1455 callback flow; + * `device` runs the deviceauth grant, which needs no local browser or listener + * and is the only workable path on a headless or remote hub (#3366). + */ +export type ChatGPTLoginFlow = "browser" | "device"; + +export async function loginChatGPT( + ctrl: OAuthController, + opts?: { forceLogin?: boolean; flow?: ChatGPTLoginFlow }, +): Promise { + if (opts?.flow === "device") { + // Imported lazily so the callback flow does not pay for a module it never uses. + const { loginChatGPTDevice } = await import("./chatgpt-device"); + return loginChatGPTDevice(ctrl); + } const flow = new ChatGPTOAuthFlow(ctrl); if (opts?.forceLogin) flow.forceLogin = true; return flow.login(); diff --git a/src/oauth/index.ts b/src/oauth/index.ts index 868c4f9b99c..3ae6f4c01f4 100644 --- a/src/oauth/index.ts +++ b/src/oauth/index.ts @@ -34,7 +34,7 @@ import { loginXai, refreshXaiToken, XAI_LOCAL_CLI_DETACH_WARNING, XaiTokenReques import { ANTHROPIC_OAUTH_BETA, AnthropicTokenError, loginAnthropic, refreshAnthropicToken } from "./anthropic"; import { loginKimi, refreshKimiToken } from "./kimi"; import { loginNous, NousTokenError, refreshNousToken, clearNousRefreshIntent, RefreshIntentIOError } from "./nous"; -import { loginChatGPT, refreshChatGPTToken } from "./chatgpt"; +import { loginChatGPT, refreshChatGPTToken, type ChatGPTLoginFlow } from "./chatgpt"; import { loginAntigravity, refreshAntigravityToken } from "./google-antigravity"; import { loginCursor, refreshCursorToken } from "./cursor"; import { loginGithubCopilot, refreshGithubCopilotToken, validateCopilotApiBaseUrl } from "./github-copilot"; @@ -161,7 +161,17 @@ function verdictKey(p:string,a:string,c:OAuthCredentials){return `${p}\0${a}\0${ function cached(p:string,a:string,c:OAuthCredentials,now:()=>number){const k=verdictKey(p,a,c),u=permanentRefreshFailures.get(k);if(u===undefined)return false;if(u<=now()){permanentRefreshFailures.delete(k);return false;}return true;} export function sweepExpiredXaiPermanentFailureVerdicts(now=Date.now()):number{let removed=0;for(const[key,until]of permanentRefreshFailures){if(until>now)continue;permanentRefreshFailures.delete(key);removed+=1;}return removed;} -export interface LoginOpts { forceLogin?: boolean; /** When set, persist into this account slot and require matching identity. */ reauthAccountId?: string } +export interface LoginOpts { + forceLogin?: boolean; + /** When set, persist into this account slot and require matching identity. */ + reauthAccountId?: string; + /** + * ChatGPT only: `device` selects the deviceauth grant instead of the + * localhost:1455 callback flow, for hosts with no browser or no loopback + * listener (#3366). Ignored by every other provider. + */ + flow?: ChatGPTLoginFlow; +} export interface LoginFlowLifecycle { /** Runs after background credential/config persistence settles, before status becomes done. */ @@ -282,7 +292,7 @@ export const OAUTH_PROVIDERS: Record = { defaultRefreshPolicy: "lazy-only", }, chatgpt: { - login: loginChatGPT, + login: (ctrl, opts) => loginChatGPT(ctrl, { forceLogin: opts?.forceLogin, flow: opts?.flow }), refresh: (rt) => refreshChatGPTToken(rt), providerConfig: { adapter: "openai-responses", baseUrl: "https://chatgpt.com/backend-api/codex", authMode: "forward" as const }, defaultModel: "gpt-5.4", diff --git a/src/oauth/log.ts b/src/oauth/log.ts index 2cd710d710f..b6ca199e9cc 100644 --- a/src/oauth/log.ts +++ b/src/oauth/log.ts @@ -23,6 +23,9 @@ const FORBIDDEN_NORMALIZED = new Set([ "oauth_code", "code_verifier", "clientsecret", + // Device-flow polling handle. Not a token, but it is the bearer of an + // in-flight authorization and must not be logged. + "device_auth_id", ]); function isForbiddenFieldKey(key: string): boolean { diff --git a/tests/chatgpt-device-auth.test.ts b/tests/chatgpt-device-auth.test.ts new file mode 100644 index 00000000000..5b7dbe78174 --- /dev/null +++ b/tests/chatgpt-device-auth.test.ts @@ -0,0 +1,246 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { loginChatGPT } from "../src/oauth/chatgpt"; +import { loginChatGPTDevice } from "../src/oauth/chatgpt-device"; +import type { OAuthController } from "../src/oauth/types"; + +/** + * The OpenAI deviceauth grant (#3366): the login path for a hub with no local + * browser and no listener on localhost:1455. + * + * Every test stubs `globalThis.fetch` and routes by URL, the same style as + * `tests/oauth-device-code-contract.test.ts`. + */ + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; +}); + +const USERCODE = "https://auth.openai.com/api/accounts/deviceauth/usercode"; +const DEVICE_TOKEN = "https://auth.openai.com/api/accounts/deviceauth/token"; +const OAUTH_TOKEN = "https://auth.openai.com/oauth/token"; + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +/** An id_token whose payload carries the identity the Codex pool requires. */ +function idToken(): string { + const payload = { + email: "Hub.Operator@Example.com", + "https://api.openai.com/auth": { chatgpt_account_id: "acct_device_123" }, + }; + const encode = (value: unknown): string => + Buffer.from(JSON.stringify(value)).toString("base64url"); + return `${encode({ alg: "none" })}.${encode(payload)}.sig`; +} + +interface RouteOptions { + pendingPolls?: number; + pendingStatus?: number; + interval?: unknown; + tokenBody?: unknown; + grantBody?: unknown; +} + +function routeFetch(opts: RouteOptions = {}): { urls: string[]; bodies: string[] } { + const urls: string[] = []; + const bodies: string[] = []; + let polls = 0; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; + urls.push(url); + if (typeof init?.body === "string") bodies.push(init.body); + if (url === USERCODE) { + return jsonResponse({ + device_auth_id: "auth-id-opaque", + user_code: "ABCD-EFGH", + ...(opts.interval === undefined ? {} : { interval: opts.interval }), + }); + } + if (url === DEVICE_TOKEN) { + if (polls < (opts.pendingPolls ?? 0)) { + polls += 1; + return jsonResponse({}, opts.pendingStatus ?? 403); + } + return jsonResponse( + opts.grantBody ?? { authorization_code: "auth-code", code_verifier: "server-verifier" }, + ); + } + if (url === OAUTH_TOKEN) { + return jsonResponse( + opts.tokenBody ?? { + access_token: "access-value", + refresh_token: "refresh-value", + id_token: idToken(), + expires_in: 3600, + }, + ); + } + throw new Error(`unexpected fetch: ${url}`); + }) as typeof fetch; + return { urls, bodies }; +} + +describe("ChatGPT device auth", () => { + test("surfaces the fixed verification URL and the human code", async () => { + routeFetch(); + let seen: { url?: string; instructions?: string; deviceCode?: string } | undefined; + await loginChatGPTDevice({ onAuth: info => { seen = info; } }); + + expect(seen?.url).toBe("https://auth.openai.com/codex/device"); + expect(seen?.deviceCode).toBe("ABCD-EFGH"); + expect(seen?.instructions).toContain("ABCD-EFGH"); + // The opaque polling handle must never reach a rendered surface. + expect(JSON.stringify(seen)).not.toContain("auth-id-opaque"); + }); + + test.each([403, 404])("treats %i as pending and keeps polling", async status => { + // interval 0.0001s floors to the 1s minimum, so two pending polls would + // sleep two real seconds. Assert the wait instead of paying for it. + const calls = routeFetch({ pendingPolls: 2, pendingStatus: status, interval: 0.001 }); + const started = Date.now(); + const creds = await loginChatGPTDevice({}); + + expect(calls.urls.filter(url => url === DEVICE_TOKEN)).toHaveLength(3); + expect(creds.access).toBe("access-value"); + // Two pending polls at the 1s floor: proves the interval is honored rather + // than collapsed to an immediate retry. + expect(Date.now() - started).toBeGreaterThanOrEqual(1_900); + }); + + test("does not accept a grant that arrives after the deadline", async () => { + const realNow = Date.now; + let clock = realNow(); + Date.now = () => clock; + try { + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; + if (url === USERCODE) { + return jsonResponse({ device_auth_id: "auth-id-opaque", user_code: "ABCD-EFGH" }); + } + if (url === DEVICE_TOKEN) { + // The poll itself outlives the 15-minute grant. + clock += 15 * 60 * 1000 + 1; + return jsonResponse({ authorization_code: "auth-code", code_verifier: "server-verifier" }); + } + throw new Error("token exchange must not be reached"); + }) as typeof fetch; + + await expect(loginChatGPTDevice({})).rejects.toThrow("expired"); + } finally { + Date.now = realNow; + } + }); + + test("accepts the upstream 'usercode' spelling", async () => { + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; + if (url === USERCODE) { + return jsonResponse({ device_auth_id: "auth-id-opaque", usercode: "WXYZ-1234" }); + } + if (url === DEVICE_TOKEN) { + return jsonResponse({ authorization_code: "auth-code", code_verifier: "server-verifier" }); + } + return jsonResponse({ access_token: "access-value", expires_in: 3600 }); + }) as typeof fetch; + + let seen: { deviceCode?: string } | undefined; + await loginChatGPTDevice({ onAuth: info => { seen = info; } }); + expect(seen?.deviceCode).toBe("WXYZ-1234"); + }); + + test("coerces a string interval and clamps an overflowing one", async () => { + // A raw setTimeout above ~2^31 ms fires immediately, which would turn a + // corrupt interval into a hot loop against an auth endpoint. + const calls = routeFetch({ pendingPolls: 3, interval: "999999999" }); + const started = Date.now(); + const abort = new AbortController(); + setTimeout(() => abort.abort("stop"), 60); + + await loginChatGPTDevice({ signal: abort.signal }).catch(() => {}); + + expect(Date.now() - started).toBeLessThan(5_000); + // One poll, then a long clamped wait — not a spin. + expect(calls.urls.filter(url => url === DEVICE_TOKEN).length).toBeLessThanOrEqual(2); + }); + + test("rejects a token response with no access token", async () => { + routeFetch({ tokenBody: { refresh_token: "refresh-value", expires_in: 3600 } }); + + await expect(loginChatGPTDevice({})).rejects.toThrow( + "ChatGPT token response missing access token", + ); + }); + + test("exchanges the server-issued grant at the device callback URI", async () => { + const calls = routeFetch(); + await loginChatGPTDevice({}); + + const exchange = calls.bodies.find(body => body.includes("grant_type=authorization_code")); + expect(exchange).toBeDefined(); + const params = new URLSearchParams(exchange ?? ""); + expect(params.get("code")).toBe("auth-code"); + // The verifier comes from the poll response; we never generate one here. + expect(params.get("code_verifier")).toBe("server-verifier"); + expect(params.get("redirect_uri")).toBe("https://auth.openai.com/deviceauth/callback"); + }); + + test("carries the account identity the Codex pool requires", async () => { + routeFetch(); + const creds = await loginChatGPTDevice({}); + + // A credential with no accountId is rejected at pool admission, so wire + // success alone would not prove the flow is usable. + expect(creds.accountId).toBe("acct_device_123"); + expect(creds.email).toBe("hub.operator@example.com"); + expect(creds.refresh).toBe("refresh-value"); + }); + + test("rejects a malformed grant without reflecting the response body", async () => { + routeFetch({ grantBody: { authorization_code: "auth-code" } }); + + await expect(loginChatGPTDevice({})).rejects.toThrow( + "ChatGPT device authorization response missing required fields", + ); + }); + + test("reports a terminal poll failure by status only", async () => { + globalThis.fetch = (async (input: RequestInfo | URL) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; + if (url === USERCODE) { + return jsonResponse({ device_auth_id: "auth-id-opaque", user_code: "ABCD-EFGH" }); + } + return new Response("upstream said something with a secret in it", { status: 500 }); + }) as typeof fetch; + + const error = await loginChatGPTDevice({}).catch((err: Error) => err); + expect(String(error)).toContain("HTTP 500"); + expect(String(error)).not.toContain("secret"); + }); + + test("stops when the controller aborts", async () => { + routeFetch({ pendingPolls: 50, interval: 0.001 }); + const abort = new AbortController(); + const ctrl: OAuthController = { + onAuth: () => abort.abort("observed"), + signal: abort.signal, + }; + + await expect(loginChatGPTDevice(ctrl)).rejects.toThrow(/cancelled|abort/i); + }); + + test("loginChatGPT routes flow:device to the device grant", async () => { + const calls = routeFetch(); + // No callback server is started: reaching the usercode endpoint at all + // proves the browser flow was not selected. + const creds = await loginChatGPT({}, { flow: "device" }); + + expect(calls.urls[0]).toBe(USERCODE); + expect(creds.accountId).toBe("acct_device_123"); + }); +});