Repository navigation
fix(sse): stop leaking a foreign combo/pool account's Codex quota headers (#14116) - #14465
Merged
Merged
Conversation
…ders (#14116) buildStreamingResponseHeaders() forwarded the x-codex-*-used-percent/-reset/ -window/-credits/-plan-type quota headers unconditionally, with no notion of which account actually served the request vs which one the caller pinned or requested. When combo/pool routing served a response through a sibling account, that account's quota leaked to the caller. Thread the caller's pinned/requested connection id (forcedConnectionId) from src/sse/handlers/chat.ts through chatHelpers.ts and chatCore.ts down to the header-assembly chokepoint, and compare it against the connection that actually served the response. When combo routing served a foreign account (pinned/requested connection differs from the one that served it), strip the Codex quota headers before they reach the caller. The direct path (no combo) and unpinned combo requests keep today's forwarding behavior unchanged. An earlier attempt (#13638) tried an unconditional strip, which would have broken the direct-path regression guard for #10315 — this fix is conditional on a proven account mismatch instead. Refs #14116
shubhayu-dev
approved these changes
Sep 22, 2026
shubhayu-dev
left a comment
Contributor
There was a problem hiding this comment.
Merging as approved
# Conflicts: # config/quality/file-size-baseline.json
diegosouzapw
added a commit
that referenced
this pull request
Sep 24, 2026
… key compared without timing-safe check (#14485) (#14571) Merge-batch 2026-09-23 (PRs do mantenedor, Trilha B). A reconciliação com o tip atual de `release/v3.8.51` foi feita no branch da PR; o detalhe está na seção "Rework (merge-batch 2026-09-23)" do corpo, quando existe. - #14571: a política de API key passa a rodar só quando o divert self-hosted está configurado (antes rodava 2x em toda request cloud). Prova red→green: o teste "cloud request runs the key policy ONCE" dava 429 na versão anterior e agora 5/5 passam; 29/29 no total. - #14465: re-medido depois do merge do tip. chat.ts 2560→2561 e chatHelpers.ts 1257→1258 (+1 cada, plumbing de `forcedConnectionId`), com anotação datada dentro de `frozen`. 11/11 testes focados (inclui o guard #5849). - #14467: reconciliada depois da #14468 (suno). Contagem de providers regerada = 358 (`gen:provider-reference` + `check:provider-consistency`); REMOVED_PROVIDERS/blocklist com as duas entradas; tripwire de prefixos reservados re-medido em 412; AGENTS.md/llm.txt mudam só o número (aprovado pelo dono). 57/57 testes focados. - Em todas: `typecheck:core` mostra só o herdado `cliproxyAccountHealth.ts:157`, e o único vermelho de `check:open-sse-typecheck` é o herdado `auggie.ts` (#14547).
diegosouzapw
added a commit
that referenced
this pull request
Sep 24, 2026
…nt to validate rewrite (#14217) (#14467) Merge-batch 2026-09-23 (PRs do mantenedor, Trilha B). A reconciliação com o tip atual de `release/v3.8.51` foi feita no branch da PR; o detalhe está na seção "Rework (merge-batch 2026-09-23)" do corpo, quando existe. - #14571: a política de API key passa a rodar só quando o divert self-hosted está configurado (antes rodava 2x em toda request cloud). Prova red→green: o teste "cloud request runs the key policy ONCE" dava 429 na versão anterior e agora 5/5 passam; 29/29 no total. - #14465: re-medido depois do merge do tip. chat.ts 2560→2561 e chatHelpers.ts 1257→1258 (+1 cada, plumbing de `forcedConnectionId`), com anotação datada dentro de `frozen`. 11/11 testes focados (inclui o guard #5849). - #14467: reconciliada depois da #14468 (suno). Contagem de providers regerada = 358 (`gen:provider-reference` + `check:provider-consistency`); REMOVED_PROVIDERS/blocklist com as duas entradas; tripwire de prefixos reservados re-medido em 412; AGENTS.md/llm.txt mudam só o número (aprovado pelo dono). 57/57 testes focados. - Em todas: `typecheck:core` mostra só o herdado `cliproxyAccountHealth.ts:157`, e o único vermelho de `check:open-sse-typecheck` é o herdado `auggie.ts` (#14547).
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
buildStreamingResponseHeaders()(open-sse/handlers/chatCore/responseHeaders.ts) forwarded thex-codex-*-used-percent/-reset/-window/-credits/-plan-typequota headers unconditionally, with nonotion of which account actually served the request vs which one the caller pinned/requested. When
combo/pool routing served a response through a sibling account, that account's quota leaked to the
caller.
An earlier attempt (#13638, closed unmerged) tried an unconditional strip of these headers, which
would have broken the direct-path regression guard for #10315 (the direct path unambiguously owns
its own quota headers and must keep forwarding them). This PR strips them only on a proven
account mismatch instead.
Fix
open-sse/handlers/chatCore/responseHeaders.ts: extracted the existingx-codex-*quota-headermatch into an exported
isCodexAccountQuotaHeader()predicate (shared by the forwarding-priorityboost and the new strip step), and added
isForeignComboAccountResponse()— true only whenisCombo && requestedConnectionId && selectedConnectionId && requestedConnectionId !== selectedConnectionId.buildStreamingResponseHeaders()now drops quota-header candidates whenthat predicate is true, before they enter the forwarding budget.
open-sse/handlers/chatCore/streamingResponseHeaders.ts: threadsisCombo/requestedConnectionId/selectedConnectionIdthroughassembleStreamingResponseHeaders().open-sse/handlers/chatCore.ts: added an optionalforcedConnectionIdparam tohandleChatCore,passed through to the header-assembly call site alongside
isComboandcredentials?.connectionIdasselectedConnectionId.src/sse/handlers/chatHelpers.ts/src/sse/handlers/chat.ts: thread the caller'spinned/requested connection id (
forcedConnectionId) down from the existingrequestedConnectionId/hasForcedConnectionplumbing intoexecuteChatWithBreaker()→handleChatCore().The direct path (no combo) and unpinned combo requests (no explicit connection pin) keep today's
forwarding behavior unchanged — the strip only triggers on a proven mismatch between the caller's
own pinned/requested connection and the connection that actually served the response.
Regression test
tests/unit/codex-quota-header-leak-14116.test.ts(new file, 3 cases):Gates run
node --import tsx/esm --test tests/unit/codex-quota-header-leak-14116.test.ts→ 3/3 passnpm run typecheck:core→ exit 0node scripts/check/check-open-sse-typecheck.mjs→ exit 0npx eslint --suppressions-location config/quality/eslint-suppressions.json <all 6 changed files>→ exit 0node scripts/check/check-file-size.mjs→ OK (see Rebaseline section below)node scripts/check/check-complexity-ratchets.mjs --base-ref origin/release/v3.8.51→ OK (new-code violations: complexity 0, cognitive 0, both baseline-neutral on the 5 touched files)node scripts/check/check-changelog-integrity.mjs→ not evaluated as a real signal here: this branch was intentionally NOT rebased onto the current release tip (7 commits behind, per instructions), so the local CHANGELOG.md is stale relative toorigin/release/v3.8.51and the script reports 474 bullets "missing" that are simply later, unrelated merges. This PR does not touchCHANGELOG.md(only adds achangelog.d/fixes/fragment, per convention) — 0 bullets removed by this diff. CI evaluates the actual PR-merge result against the current base, which will be unaffected by this branch's lag.npm run check:public-creds— not run; no public-cred literal lines were moved or added.Existing tests
Ran every test file referencing the touched symbols/files (
buildStreamingResponseHeaders,assembleStreamingResponseHeaders,isCodexAccountQuotaHeader,responseHeaders.ts,middleware-header-strip):tests/unit/13601-header-drop-count-surfaced.test.ts— 3/3tests/unit/chatcore-header-drop-warn-dedupe-10315.test.ts— 5/5tests/unit/chatcore-streaming-response-headers.test.ts— 5/5tests/unit/codex-turn-state.test.ts— 9/9tests/unit/middleware-header-strip-5849.test.ts(the fix(backend): deduplicate repeated upstream-header budget warnings #10315 direct-path regression guard) — 9/9, all green, unchangedtests/unit/omniroute-decision-header.test.ts— 9/9tests/unit/chatcore-translation-paths.test.ts— 79/79tests/unit/g13-combo-chatcore-golden.test.ts(public-behavior golden lock onchatCore.ts) — 3/3None were modified — all passed as-is against the fix.
Rebaseline (needs owner approval)
config/quality/file-size-baseline.jsongained one entry,_rebaseline_2026_09_21_14116_codex_quota_header_leak, raising two frozen caps by +1 line each:src/sse/handlers/chat.ts2547→2548 andsrc/sse/handlers/chatHelpers.ts1253→1254. I re-auditedthis before opening the PR and could not reduce it further: each is one field added to an existing
multi-line call-site object literal (
forcedConnectionId: hasForcedConnection ? forcedConnectionId : nullinchat.ts, and one destructured param + one passthrough field inchatHelpers.ts,already offset there by compacting an adjacent 3-line comment to 2 lines).
open-sse/handlers/chatCore.tsstayed within its existing cap (6282 vs 6287) without any adjustment. The new predicate/strip logic
itself (
isCodexAccountQuotaHeader,isForeignComboAccountResponse) lives entirely in thenon-frozen
open-sse/handlers/chatCore/responseHeaders.ts, so only the two thin plumbing callsites needed the +1/+1.
Prior art
Credit to @fenix007 for the original report (#13638, closed unmerged — the unconditional-strip
attempt this PR avoids repeating) and to @shubhayu-dev, whose parallel PR #14137 (against #13638,
still open) independently arrived at the same
isForeignAccount-style framing.Closes #14116
Plan-file:
_tasks/pipeline/bugs/2-implementing/14116-fix-sse-codex-quota-headers-leak-the-selected-pool-combo-accou.plan.md