fix(ai): fail closed when a models endpoint validates a key on status alone - #4586
fix(ai): fail closed when a models endpoint validates a key on status alone#4586Yeachan-Heo wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Exact-head review requested from @probepark, @HaD0Yun, and @IYENTeam for Canonical binary-diff digest: Adversarial evidence on this head:
The PR body carries a canonical — |
|
Current Re-evaluation against
Reconstruction is therefore not required; rebasing would only churn the head and stale clean disjoint evidence. The current exact-head run remains the applicable product run. Merge stays blocked pending its native-build completion and a fresh authorized exact-head approval. — |
|
Fresh exact-head review is required for Generation-1 adversarial QA found that a real dataless 204 failed closed but its new error text incorrectly said Current exact-head evidence:
@probepark @HaD0Yun @IYENTeam: please review and approve only this exact SHA. The canonical verdict remains — |
33 commits over 0.13.2: hotfixes (#4437 #4424 #4481 #4446 #4453 #4452), session/storage resilience (#4396 #4411 #4373), security/stability (#4385 Synthetic models-endpoint validation, #4302 image-gen redaction), agent escaped-non-ASCII managed recovery (#4515 release variant), Kiro OAuth (#4304), Muse Spark 1.2, xAI + Grok CLI 4.6, Ouroboros/iTerm2 pet (#4468 #4499), integration docs. Release is HELD: no version bump, tag, or publish; binary stays gjc/0.13.2 until bun run release cuts 0.13.3 after dogfood verification. Pending before the cut: integration groups 3-4 (#4421 #4450 #4470 #4495), validation evidence, PR #4586 backport decision.
|
Exact-head product CI is now green for Run
The sole failing job is the intentionally blocked @probepark @HaD0Yun @IYENTeam: one exact-head approval is the only remaining merge prerequisite. Digest remains — |
9ecf1e3 to
dec4937
Compare
|
PR #4586 has been authoritatively reconciled onto current
Every run, review, and approval attached to — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 9ecf1e39f
fix(ai): fail closed when a models endpoint validates a key, head commit "report actual models
response status". 3 files, +187.
$ bun test <the PR's touched suites>
12 pass 0 fail
Why the direction is right
Treating a models-endpoint response as key validation is only safe if a non-success status is
treated as "unknown", not "fine". Failing open here means a provider whose /models endpoint is
down, rate-limited, or returning a 403 gets recorded as a working credential, and the failure
resurfaces later as a confusing runtime error on the first real request instead of at validation
time.
Reporting the actual response status rather than collapsing it to a boolean is the part that makes
the fail-closed decision reviewable - a 401 and a 503 mean different things about the key, and a
caller that only sees false cannot distinguish "this key is wrong" from "we could not tell".
merge-approved.
Reviewed by @probepark - method: fresh-worktree run at the exact head.
dec4937 to
90dbafa
Compare
|
Current PR #4586 has been reconciled onto exact base
Review — |
|
Exact-head replacement CI is product-green for Authoritative run
The sole failure is the intentional Fresh exact-head evidence is also clean: architect CLEAR/CLEAR/CLEAR APPROVE, baseline 12/12, adversarial 66/66, affected provider validation 44/44, isolated login/OAuth sweep 212/212, AI package check, and Bun bundle. Canonical digest: @probepark @HaD0Yun @IYENTeam: one fresh write-authorized non-author APPROVED review at exact head — |
… alone
validateApiKeyAgainstModelsEndpoint returned on any response.ok without
reading the body, so a captive portal, misrouting proxy, or broken gateway
answering 200 with an HTML page silently validated and stored an API key that
was never actually checked. Found by an adversarial QA harness during the
0.13.3 release review: stubbed 200 responses with malformed JSON and with a
dataless JSON object were both accepted.
A 200 now requires a body that parses as JSON and carries a recognizable
model list: the OpenAI-compatible `data` array, a gateway `models` array, or
a bare array. Anything else fails with an actionable error naming the reason.
Upstream bodies echoed into validation errors are bounded to 200 characters
on both validators.
Constraint: all nine models-endpoint consumers are OpenAI-compatible /models URLs, so requiring a list shape is safe
Constraint: an empty model list still validates -- plan-restricted keys list zero models yet authenticate
Rejected: validating the chat-completions probe body too | its response shape is not a typed contract; separate change if wanted
Rejected: requiring exactly {object:"list",data:[]} | gateways answer with bare arrays and models fields
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: 8 focused cases incl. captive-portal 200, dataless 200, bounded 500 echo, network failure; synthetic-login suite; 60-suite oauth sweep with zero introduced failures vs base
Not-tested: live provider endpoints
The models-endpoint hardening claimed bounded upstream details across both validation strategies, but chat-completions validation still echoed an unbounded response body. Bound that path too and add explicit malformed JSON and oversized 200-body coverage. Lore-id: pr4586-bound-echoes Constraint: models-endpoint validation must fail closed without rejecting empty authenticated model arrays Tested: 43 provider validation and login tests; 212 isolated login/oauth tests; AI package check and Bun bundle Scope-risk: narrow Reversibility: easy
Successful-body validation covers every 2xx response, but its new refusal messages hard-coded 200. A real 204 therefore failed closed with misleading diagnostics. Interpolate the actual status and lock the 204 case. Lore-id: pr4586-models-status Constraint: every dataless successful response must fail closed with accurate status evidence Tested: 44 affected provider tests; 212 isolated login/oauth tests; AI package check and Bun bundle Scope-risk: narrow Reversibility: easy
90dbafa to
99afd9e
Compare
|
PR #4586 has been reconstructed onto exact current
Every older review, approval, verdict, and CI receipt is stale. @probepark @HaD0Yun @IYENTeam: please review and approve only exact head — |
|
Exact-head replacement CI is product-green for Run Fresh current-head cohort evidence is clean: architect CLEAR/CLEAR/CLEAR APPROVE; baseline 12/12; adversarial 66/66; affected providers 44/44; isolated login/OAuth 212/212; AI check/build clean. Digest recomputes to @probepark @HaD0Yun @IYENTeam: an APPROVED review bound to exact commit — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 99afd9ec8
Head moved since my approval of dec49376a; re-verified at the exact head.
Differential
# base 91a35114d, with this head test files applied
5 pass 7 fail
# head 99afd9ec8
12 pass 0 fail
Seven failures on base - stronger evidence than the head-only run I had last time.
Substantive review stands: treating a models-endpoint response as key validation is only safe if a
non-success status means "unknown" rather than "fine". Reporting the actual status rather than
collapsing to a boolean is what makes the fail-closed decision reviewable, since a 401 and a 503 say
different things about the key.
merge-approved at 99afd9ec8.
Reviewed by @probepark - method: fresh-worktree run at the exact head, clean base worktree with the head test files to prove the differential.
2 participants
Problem
validateApiKeyAgainstModelsEndpointreturned on anyresponse.okwithout reading the body. A captive portal, misrouting proxy, or broken gateway answering successfully with HTML, malformed JSON, or dataless JSON could silently validate and store an unchecked API key.Fix
A successful models response must parse as JSON and carry a recognizable model list: an OpenAI-compatible
dataarray, a gatewaymodelsarray, or a bare array. HTML, malformed JSON, and dataless JSON fail closed. Empty authenticated arrays remain valid. Upstream response diagnostics are bounded to 200 characters in both models-endpoint and chat-completions validators, and successful non-200 2xx refusals report the actual status.Affects Synthetic, DeepSeek, DeepInfra, Fireworks, BizRouter, NanoGPT, OpenGateway, ZenMux, and Fugu.
Fresh current-dev evidence
99afd9ec84da32d9e8ef8575aeee1834759fe65891a35114d1263dd8422afa1c3e5a3b65d60ff724(dev)packages/ai/CHANGELOG.md,packages/ai/src/utils/oauth/api-key-validation.ts, andpackages/ai/test/api-key-validation.test.ts(+187/-2). No shared current-dev TDZ repair was copied into this PR.sha256:329abc6ffaad5149aa48e1de3577a55619f2df668d5f6c1dbf1bda77507ac00fbun --cwd=packages/ai run check: clean.bun build packages/ai/src/index.ts --target bun: clean.gajae.pr-review-verdict.v1 needs-human sha256:329abc6ffaad5149aa48e1de3577a55619f2df668d5f6c1dbf1bda77507ac00f reviewer:human reviewer-id:pending evidence:exact-head-99afd9ec84-current-dev-91a35114-rebase-clean-three-file-delta-local-validation-44-auth-sweep-212-ai-check-build-clean-fresh-ci-and-non-author-review-pending
—
[repo owner's gaebal-gajae (clawdbot) 🦞]