fix: prefer apikey upstream routing without login#361
fix: prefer apikey upstream routing without login#361SsuJojo wants to merge 3 commits intoicebear0828:masterfrom
Conversation
Skip local Codex account auth when a request is routed directly to an upstream API-key provider, so configured apikeys work even if no local account is logged in.
Handle colon-containing model IDs safely in upstream routing and cover the proxy-key requirement for direct upstream requests.
Classify chat models explicitly so non-Codex requests use configured upstream API keys, Codex requests prefer account-backed routing, and exhausted Codex requests can fall back to configured upstream keys without silently routing unknown models to Codex.
|
@codex review |
icebear0828
left a comment
There was a problem hiding this comment.
Review
Overall direction is right — promoting API-key routes above Codex auth is the correct fix. A few issues to address before merging.
Bug: auth bypass for unknown models in messages.ts, gemini.ts, responses.ts
isCodexModel(model) now returns false for models that don't match any route (because resolveMatch returns not-found, not codex). So the guard:
const allowUnauthenticated = !!(upstreamRouter && !upstreamRouter.isCodexModel(model));evaluates to true for completely unknown models, bypassing auth before the request ultimately fails somewhere downstream. chat.ts handles not-found explicitly with a 404 — the other three routes should do the same, and the allowUnauthenticated check should be kind === "api-key" || kind === "adapter", not !isCodexModel.
Dead code in chat.ts — the hasApiKeyModel fallback after a codex resolve
Lines 675–682 (after the routeMatch.kind === "codex" path is entered):
if (!accountPool.isAuthenticated()) {
if (upstreamRouter?.hasApiKeyModel(req.model)) { // ← can never be true here
return handleDirectRequest(…);
}
…
}resolveMatch checks the api-key pool before classifying a model as codex. If a pool entry matched, the result would be api-key, not codex. So hasApiKeyModel() is always false inside this branch. Same dead code at lines 696–703. Remove both; they obscure the real flow.
hasKnownCodexModel in chat.ts is inconsistent with isKnownCodexModel in the router
// chat.ts (used when upstreamRouter is undefined):
function hasKnownCodexModel(model: string): boolean {
return !!aliases[model] || !!getModelInfo(model);
}
// upstream-router.ts:
private isKnownCodexModel(model: string): boolean {
…
if (/^(gpt|o\d|codex)/i.test(trimmed)) return true; // missing in chat.ts
…
}A model like gpt-4o would be treated as not-found by chat.ts when there's no router configured, but as codex by the router. Extract to a shared utility or at least make the two copies identical.
Proxy API key silently bypassed for api-key/adapter routes
chat.ts skips the proxy_api_key check entirely when routeMatch.kind === "api-key" || kind === "adapter". The test explicitly validates this. If the proxy is exposed behind a key, this allows anyone who knows an api-key model name to bypass the gate. Intentional or not, it should be either documented in a comment at that branch or the key check kept (just skip the account auth, not the proxy auth).
Minor: double blank line
chat.ts around the if (summary.active === 0) block — cosmetic but it came with this PR.
Tests only mock the router, not the real routing logic
The new upstream-auth-bypass.test.ts injects a pre-built mock router (resolveMatch: vi.fn(() => ({ kind: "adapter" }))), so it doesn't actually exercise the isKnownCodexModel heuristic, the pool lookup, or the hasApiKeyModel interaction. Worth adding at least one integration-style test that wires up a real UpstreamRouter with an api-key pool entry to confirm the full path.
|
Closed in favor of #366 which includes all your commits (with original authorship preserved) plus two additional bug fixes identified during review. Your work is in master — thanks for the contribution! |
Summary
Test plan