fix(ai): fail closed when a models endpoint validates a key on status alone - #4586
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 — |
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.
… 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
The original PR's immutable base predates the 0.14.0 release cut.\nMove its changelog entry back to Unreleased after rebasing so released history\nremains immutable. Lore-id: pr4586-reconcile-changelog\nConfidence: high\nScope-risk: narrow\nReversibility: easy\nTested: focused API-key validation test
99afd9e to
4dba501
Compare
|
@probepark Exact-head review requested for — |
#4636 and this branch both appended to packages/ai/CHANGELOG.md Unreleased. Keep both entries in dev order so neither provider fix is dropped. Confidence: high Scope-risk: narrow Reversibility: easy Tested: changelog only, no code paths touched
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head c64d0f5. independent-architect-review-accepts-openai-data-gateway-models-and-bare-array-shapes-rejects-html-and-dataless-success-no-configured-provider-validates-on-bare-status-plus-maintainer-changelog-conflict-resolution
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head 66eca06. independent-architect-review-accepts-openai-data-gateway-models-and-bare-array-shapes-rejects-html-and-dataless-success-no-configured-provider-validates-on-bare-status-plus-maintainer-changelog-conflict-resolution
… alone (Yeachan-Heo#4586) * fix(ai): fail closed when a models endpoint validates a key on status 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 * fix(ai): bound validation response echoes consistently 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 * fix(ai): report actual models response status 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 * fix(ai): keep models validation note unreleased The original PR's immutable base predates the 0.14.0 release cut.\nMove its changelog entry back to Unreleased after rebasing so released history\nremains immutable. Lore-id: pr4586-reconcile-changelog\nConfidence: high\nScope-risk: narrow\nReversibility: easy\nTested: focused API-key validation test --------- Co-authored-by: Yeachan-Heo <yeachan-heo@gajae.dev> Co-authored-by: Yeachan Heo <yeachan.heo@gmail.com> Co-authored-by: probe <re2rar@gmail.com> Co-authored-by: probepark <probepark@users.noreply.github.com> (cherry picked from commit 9f29ee5)
2 participants
What
Rebased the API-key models-endpoint validation fix onto current
devand retained the intended three-file delta.Why
A successful HTTP status alone is not proof that an API key reached a models API. Captive portals, proxies, and broken gateways can return 200 with HTML or unrelated JSON.
Testing
bun test packages/ai/test/api-key-validation.test.ts— 12 passedbun test packages/ai/test/auth-storage-api-key-login.test.ts— 3 passedbun --cwd=packages/ai run check— passedbun build packages/ai/src/index.ts --target bun --outdir /tmp/pr-4586-ai-build— passedgit diff --check origin/dev...HEAD— passedpackages/ai/CHANGELOG.md,packages/ai/src/utils/oauth/api-key-validation.ts,packages/ai/test/api-key-validation.test.tsGJC verdict
The prior
probeparkapproval targets99afd9ec84da32d9e8ef8575aeee1834759fe658, not the refreshed exact head. Current product CI is green. The sole remaining gate is an authenticated non-author review bound to this exact head, base, and digest.devbun checkpasses (not run; bounded package check passed)—
[repo owner's gaebal-gajae (clawdbot) 🦞]