Skip to content

Commit ad486c4

Browse files
ndycodendycode
authored
merge: land rollup fix branch from PR #432
Merge the reviewed rollup fix branch after deep audit, full verification battery, and real pack/install smoke test. This lands the integrated fix set from PRs #414-#429 and #431 via one reviewed branch, including the two rollup-only integration commits: - 7f50455 fix(rollup): add missing numeric tracker cleanup helpers from PR #419 - 056ad18 test(rollup): restore auth-list empty-state expectation to current behavior Rationale: the rollup branch was the only branch deep-audited end-to-end and pack/install smoke-tested as a complete artifact. Baseline failures in codex-bin-wrapper / benchmark-runtime-path-script already exist on main and are not regressions from this merge.
2 parents 3f1c1fe + 056ad18 commit ad486c4

32 files changed

Lines changed: 1721 additions & 4335 deletions

‎lib/accounts.ts‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -866,6 +866,16 @@ export class AccountManager {
866866
): void {
867867
account.lastSwitchReason = reason;
868868
this.currentAccountIndexByFamily[family] = account.index;
869+
// HI-02: keep cursorByFamily in lockstep so subsequent round-robin
870+
// passes resume AFTER the just-switched account, matching the
871+
// convention used in getCurrentOrNextForFamilyHybrid / the
872+
// getCurrentOrNextForFamily inner loop. Without this, the cursor
873+
// still points at the pre-switch position and the next selection
874+
// can re-pick or skip the freshly marked account.
875+
const count = this.accounts.length;
876+
if (count > 0) {
877+
this.cursorByFamily[family] = (account.index + 1) % count;
878+
}
869879
}
870880

871881
/**
@@ -900,6 +910,13 @@ export class AccountManager {
900910
return withRoutingMutex(this.routingMutexMode, () => {
901911
account.lastSwitchReason = reason;
902912
this.currentAccountIndexByFamily[family] = account.index;
913+
// HI-02: keep cursorByFamily in lockstep with the active-index
914+
// mutation so the mutex-serialized variant preserves the same
915+
// round-robin invariant as the legacy `markSwitched` path.
916+
const count = this.accounts.length;
917+
if (count > 0) {
918+
this.cursorByFamily[family] = (account.index + 1) % count;
919+
}
903920
const trackerKey = getRuntimeTrackerKey(account);
904921
const healthTracker = getHealthTracker();
905922
const tokenTracker = getTokenTracker();
@@ -1316,16 +1333,38 @@ export class AccountManager {
13161333

13171334
// Snapshot family pointers before splice so we can distinguish "was
13181335
// pointing at the removed account" from "was pointing past it".
1319-
const priorCursor: Record<ModelFamily, number> = {} as Record<ModelFamily, number>;
1320-
const priorActive: Record<ModelFamily, number> = {} as Record<ModelFamily, number>;
1336+
const priorCursor: Record<ModelFamily, number> = {} as Record<
1337+
ModelFamily,
1338+
number
1339+
>;
1340+
const priorActive: Record<ModelFamily, number> = {} as Record<
1341+
ModelFamily,
1342+
number
1343+
>;
13211344
for (const family of MODEL_FAMILIES) {
13221345
priorCursor[family] = this.cursorByFamily[family];
13231346
priorActive[family] = this.currentAccountIndexByFamily[family];
13241347
}
13251348

13261349
this.accounts.splice(idx, 1);
1350+
// Clear numeric-keyed tracker state in the shifted range. After reindex,
1351+
// any refresh-only account that moved from N to N-1 must not inherit the
1352+
// stale health/token entries that used to belong to the old numeric slot.
1353+
getHealthTracker().clearNumericKeysAtOrAbove(idx);
1354+
getTokenTracker().clearNumericKeysAtOrAbove(idx);
13271355
this.accounts.forEach((acc, index) => {
13281356
acc.index = index;
1357+
// Invalidate the cached runtime tracker key when it was keyed by
1358+
// numeric index (fallback path in getRuntimeAccountIdentityKey).
1359+
// After the splice+reindex above, a remaining account that was at
1360+
// index N (e.g. 3) may now live at index N-1 (e.g. 2); if we keep
1361+
// the previously cached numeric key, rotation/health/token state
1362+
// queries would consult the stale position and mismatch the
1363+
// current one. Identity-based string keys remain stable because
1364+
// accountId/email are not affected by array position changes.
1365+
if (typeof acc._runtimeTrackerKey === "number") {
1366+
acc._runtimeTrackerKey = undefined;
1367+
}
13291368
});
13301369

13311370
if (this.accounts.length === 0) {

‎lib/auth/auth.ts‎

Lines changed: 98 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,98 @@ function getOAuthResponseLogMetadata(
6666
return { responseType: typeof rawResponse };
6767
}
6868

69+
const OAUTH_SENSITIVE_BODY_KEYS = [
70+
"refresh_token",
71+
"refreshToken",
72+
"access_token",
73+
"accessToken",
74+
"id_token",
75+
"idToken",
76+
"codeVerifier",
77+
"token",
78+
"code",
79+
"code_verifier",
80+
] as const;
81+
82+
function scrubTokenLikeSubstrings(value: string): string {
83+
let scrubbed = value.replace(
84+
/(\b(?:refresh|access|id)[_-]?token\s*[:=]\s*)([^\s,;"'}]{8,})/gi,
85+
(_match, prefix) => `${prefix}***REDACTED***`,
86+
);
87+
scrubbed = scrubbed.replace(
88+
/\b(?:RT|AT)_ch_[A-Za-z0-9_-]{20,}\b/g,
89+
"***REDACTED***",
90+
);
91+
return scrubbed;
92+
}
93+
94+
function redactSensitiveFields(value: unknown): unknown {
95+
if (Array.isArray(value)) {
96+
return value.map((item) => redactSensitiveFields(item));
97+
}
98+
if (value !== null && typeof value === "object") {
99+
const out: Record<string, unknown> = {};
100+
const sensitiveSet = new Set<string>(OAUTH_SENSITIVE_BODY_KEYS);
101+
for (const [k, v] of Object.entries(value as Record<string, unknown>)) {
102+
if (sensitiveSet.has(k)) {
103+
out[k] = "***REDACTED***";
104+
} else {
105+
out[k] = redactSensitiveFields(v);
106+
}
107+
}
108+
return out;
109+
}
110+
if (typeof value === "string") {
111+
return scrubTokenLikeSubstrings(value);
112+
}
113+
return value;
114+
}
115+
116+
/**
117+
* Scrub opaque tokens from a raw OAuth token-endpoint response body before
118+
* interpolating it into a log message.
119+
*
120+
* Error and success responses from `/oauth/token` may contain `refresh_token`,
121+
* `access_token`, or `id_token` values. ChatGPT refresh tokens are opaque
122+
* high-entropy strings that do NOT match the logger's `TOKEN_PATTERNS`
123+
* (JWT, long hex, `sk-*`, `Bearer <x>`), so they would be written verbatim
124+
* to disk log files when a status-body string is concatenated into a
125+
* `logError` message.
126+
*
127+
* Strategy:
128+
* 1. If the body parses as JSON, walk it and mask sensitive keys.
129+
* 2. Otherwise fall back to a targeted regex scrub of `"key":"value"` and
130+
* `key=value` patterns for the known sensitive keys.
131+
*
132+
* The returned string is safe to interpolate into log messages.
133+
*/
134+
export function sanitizeOAuthResponseBodyForLog(rawBody: string): string {
135+
if (!rawBody) return rawBody;
136+
const trimmed = rawBody.trim();
137+
if (trimmed.length === 0) return rawBody;
138+
139+
if (trimmed.startsWith("{") || trimmed.startsWith("[")) {
140+
try {
141+
const parsed = JSON.parse(trimmed) as unknown;
142+
const redacted = redactSensitiveFields(parsed);
143+
return JSON.stringify(redacted);
144+
} catch {
145+
// Fall through to regex scrub for malformed JSON.
146+
}
147+
}
148+
149+
let scrubbed = rawBody;
150+
for (const key of OAUTH_SENSITIVE_BODY_KEYS) {
151+
// "key":"value" style (JSON-like text)
152+
const jsonPattern = new RegExp(`("${key}"\\s*:\\s*)"[^"]*"`, "g");
153+
scrubbed = scrubbed.replace(jsonPattern, `$1"***REDACTED***"`);
154+
// key=value style (urlencoded / query-string)
155+
const urlPattern = new RegExp(`(^|[?&\\s])(${key}=)[^&\\s]+`, "g");
156+
scrubbed = scrubbed.replace(urlPattern, `$1$2***REDACTED***`);
157+
}
158+
return scrubbed;
159+
}
160+
69161
/**
70162
* Redacts sensitive OAuth query parameters for safe logging.
71163
* Returns the original string when parsing fails.
@@ -160,12 +252,13 @@ export async function exchangeAuthorizationCode(
160252
});
161253
if (!res.ok) {
162254
const text = await res.text().catch(() => "");
163-
logError(`code->token failed: ${res.status} ${text}`);
255+
const safeText = sanitizeOAuthResponseBodyForLog(text);
256+
logError(`code->token failed: ${res.status} ${safeText}`);
164257
return {
165258
type: "failed",
166259
reason: "http_error",
167260
statusCode: res.status,
168-
message: text || undefined,
261+
message: safeText || undefined,
169262
};
170263
}
171264
const rawJson = (await res.json()) as unknown;
@@ -252,12 +345,13 @@ export async function refreshAccessToken(
252345

253346
if (!response.ok) {
254347
const text = await response.text().catch(() => "");
255-
logError(`Token refresh failed: ${response.status} ${text}`);
348+
const safeText = sanitizeOAuthResponseBodyForLog(text);
349+
logError(`Token refresh failed: ${response.status} ${safeText}`);
256350
return {
257351
type: "failed",
258352
reason: "http_error",
259353
statusCode: response.status,
260-
message: text || undefined,
354+
message: safeText || undefined,
261355
};
262356
}
263357

‎lib/auth/server.ts‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,18 @@ export function startLocalOAuthServer({
6666
"default-src 'none'; style-src 'unsafe-inline'; img-src 'self' data:; font-src 'self' data:; script-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'none'",
6767
);
6868
res.end(successHtml);
69-
const trackedServer = server as http.Server & { _lastCode?: string };
69+
const trackedServer = server as http.Server & {
70+
_lastCode?: string;
71+
_lastState?: string;
72+
};
7073
if (trackedServer._lastCode) {
7174
logWarn(
7275
"Duplicate OAuth callback received; preserving first authorization code",
7376
);
7477
return;
7578
}
7679
trackedServer._lastCode = code;
80+
trackedServer._lastState = state;
7781
} catch (err) {
7882
logError(
7983
`Request handler error: ${(err as Error)?.message ?? String(err)}`,
@@ -95,17 +99,28 @@ export function startLocalOAuthServer({
9599
pollAborted = true;
96100
server.close();
97101
},
98-
waitForCode: async () => {
102+
waitForCode: async (expectedState: string) => {
99103
const POLL_INTERVAL_MS = 100;
100104
const TIMEOUT_MS = 5 * 60 * 1000;
101105
const maxIterations = Math.floor(TIMEOUT_MS / POLL_INTERVAL_MS);
102106
const poll = () =>
103107
new Promise<void>((r) => setTimeout(r, POLL_INTERVAL_MS));
104108
for (let i = 0; i < maxIterations; i++) {
105109
if (pollAborted) return null;
106-
const lastCode = (server as http.Server & { _lastCode?: string })
107-
._lastCode;
108-
if (lastCode) return { code: lastCode };
110+
const trackedServer = server as http.Server & {
111+
_lastCode?: string;
112+
_lastState?: string;
113+
};
114+
const lastCode = trackedServer._lastCode;
115+
if (lastCode) {
116+
if (trackedServer._lastState !== expectedState) {
117+
logWarn(
118+
"Discarding OAuth callback due to state mismatch in waitForCode",
119+
);
120+
return null;
121+
}
122+
return { code: lastCode };
123+
}
109124
await poll();
110125
}
111126
logWarn("OAuth poll timeout after 5 minutes");
@@ -130,7 +145,7 @@ export function startLocalOAuthServer({
130145
);
131146
}
132147
},
133-
waitForCode: () => Promise.resolve(null),
148+
waitForCode: (_expectedState: string) => Promise.resolve(null),
134149
});
135150
});
136151
});

‎lib/codex-manager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1360,8 +1360,8 @@ async function promptManualCallback(
13601360
return null;
13611361
}
13621362
const parsed = parseAuthorizationInput(answer);
1363-
if (!parsed.code) return null;
1364-
if (parsed.state && parsed.state !== state) return null;
1363+
if (!parsed.code || !parsed.state) return null;
1364+
if (parsed.state !== state) return null;
13651365
return parsed.code;
13661366
} catch (error) {
13671367
if (isAbortError(error) || isReadlineClosedError(error)) {

0 commit comments

Comments
 (0)