fix(ai): resolve the Azure API key fallback from trusted env - #3272
Conversation
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile exact-head review — PR #3272
Terminal verdict: APPROVE / MERGE_READY
| Gate | Result |
|---|---|
| Exact head | 328352dd05ec2fbbc734cb7c2abff1c42efe2c96 (matches requested; gh headRefOid == local refs/review/pr-3272) |
| Base | origin/dev @ 2d8770169c15cd13efd54aba43ea2cddaab56a93 (PR baseOid matches) |
| Mergeability | mergeable=MERGEABLE, mergeStateStatus=CLEAN; git merge-tree vs origin/dev = 0 conflict markers |
| Exact-head terminal CI | green — 21 check-runs on this SHA: 16 success / 0 failure / 0 pending / 5 skipped. Includes test:packages/ai/test/azure-api-key-trust.test.ts, check:@gajae-code/ai, cli-smoke, native-build, ts-build matrix, gjc-state-gates, evidence producer. Dev CI run 30246561775 |
| Contributor surface | external fork 10kH:fix/azure-api-key-trust (cross-repo) |
Contract under review
Isolated residual from #3263: createClient in azure-openai-responses.ts still fell back to $env.AZURE_OPENAI_API_KEY (merged view including cwd/.env), violating the codebase rule that provider credentials must use $credentialEnv / $pickCredentialEnv.
Diff proof
- New
resolveAzureClientApiKey(apiKey): explicit non-empty caller key wins; else$credentialEnv("AZURE_OPENAI_API_KEY") createClientuses that resolver and throws the same missing-key error when neither source supplies a key- Narrow
resolveAzureClientApiKeyForTestseam only
Primary stream path already did options?.apiKey || getEnvApiKey(model.provider) || "", and getEnvApiKey("azure-openai" | "azure-openai-responses") is already $credentialEnv("AZURE_OPENAI_API_KEY"). The hole was specifically the empty-string fallback inside createClient re-reading the merged $env view — which this PR closes.
Trust tests (process isolation)
azure-api-key-trust.test.ts + probe:
- nothing supplies a key → null
- planted project
.envkey ignored - inherited key honored
- planted cannot override inherited
- explicit caller key stays ahead of environment
Proof-first (revert fails plant case) matches the single resolver change.
Hostile residual scan (non-blocking for this PR's contract)
- On this head alone, Azure endpoint still reads
$env.AZURE_OPENAI_BASE_URL/$env.AZURE_OPENAI_RESOURCE_NAME— intentional: endpoint trust is #3268's contract, not this PR's. With #3268 merged, the residual chain (project-supplied key + project-supplied host) is fully broken; this PR alone is still the correct credential-source fix and matches doctrine even before endpoint land. AZURE_OPENAI_API_VERSION/AZURE_OPENAI_DEPLOYMENT_NAME_MAPstill$env— not credential material; same defense-in-depth note as on #3268.image-gen.tsresiduals — out of file set; author correctly deferred.- Whitespace-only caller
apiKeyremains truthy and skips env — pre-existing edge; not introduced here.
Cross-PR overlap / order (critical)
- Touches the same file as #3268 (
packages/ai/src/providers/azure-openai-responses.ts).git merge-treeof #3268 vs #3272 reports 2 "changed in both" regions — landing both without rebase will conflict. - Required order: merge #3268 first, then rebase/retarget #3272 (or resolve the trivial combine: keep #3268's
$credentialEnvendpoint resolution and this PR's$credentialEnvkey fallback). - Disjoint from #3270.
This order risk is an integration note, not a reason to reject this head's isolated credential fix.
Verdict
APPROVE / MERGE_READY at exact head 328352dd05ec2fbbc734cb7c2abff1c42efe2c96.
Stated Azure API-key fallback trust boundary is correct, tested under process isolation, and CI-green. Coordinate merge after #3268 (or combine carefully) because of the shared azure provider file. Any new head SHA voids this review.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
There was a problem hiding this comment.
stale exact-head review refresh: re-affirming APPROVED at current head (no diff since last review).
328352d to
2f9c9e1
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — exact head 2f9c9e1e76328e62ab61859c3651d7039d2f22a0
Rebase-only refresh of the prior exact-head review at 328352dd05ec2fbbc734cb7c2abff1c42efe2c96 (APPROVE).
Checks performed:
- Content identity confirmed, not assumed:
diff <(git show 328352dd --format= -- packages/ai/src/providers/azure-openai-responses.ts packages/ai/test/azure-api-key-trust.test.ts packages/ai/test/fixtures/azure-api-key-probe.ts) <(git show 2f9c9e1e --format= -- packages/ai/src/providers/azure-openai-responses.ts packages/ai/test/azure-api-key-trust.test.ts packages/ai/test/fixtures/azure-api-key-probe.ts)is empty — the PR's own commit is byte-for-byte identical to the version the prior review validated. CHANGELOG.md is intentionally excluded from this file set: its diff context shifted because independently-mergeddevcommits inserted new entries above the same insertion point, which is expected rebase churn, not a content change to this PR's own line. - Rebase mechanics verified:
git merge-baseconfirms this PR carries the same single commit both before and after the rebase; every additional commit between the two heads is an independently-mergeddevcommit (own-PR-numbered), not new work folded into this PR. - Exact-head terminal CI: 16 success / 0 failure / 0 pending / 5 skipped (total 21). No pending/queued checks remain.
Prior technical analysis still applies verbatim to this head.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Adversarial exact-head review — PR #3272
Terminal verdict: MERGE_READY (held — see disposition)
| Gate | Result |
|---|---|
| Exact head | 2f9c9e1e76328e62ab61859c3651d7039d2f22a0 |
| Base | dev @ b853f15d7167a49173bc03ef7efe99c5bae8f234 |
| Mergeability | MERGEABLE / CLEAN; git merge-tree vs origin/dev = 0 conflicts |
| Exact-head CI | green — 16 pass / 0 fail / 5 skipped |
| Local focused | azure-api-key-trust → 5 pass / 0 fail |
| Local aggregate | check:@gajae-code/ai (biome 419 files + tsc --noEmit) clean |
What the change actually does
Extracts the AZURE_OPENAI_API_KEY fallback in createClient() (packages/ai/src/providers/azure-openai-responses.ts) into resolveAzureClientApiKey(), reading via $credentialEnv instead of $env.
Adversarial findings
This is a credential-exfiltration-adjacent fix, and it is the most severe of the three in kind. #3268 and #3270 concern where an authenticated request is sent; this one concerns what secret the client authenticates with. Pre-fix, $env.AZURE_OPENAI_API_KEY meant a repository's cwd/.env could supply the key the Azure client uses. Combined with an endpoint redirect, that is attacker-chosen credential to attacker-chosen host. Worth noting the two halves of that chain live in #3268 and this PR — both should land.
Consistency argument checks out. packages/utils/src/env.ts:92-94 explicitly documents that provider credential resolution must not use $env and must use $credentialEnv/$pickCredentialEnv. This fallback was violating a rule the codebase had already written down. The fix brings it into line rather than inventing new policy.
Control-flow refactor is behavior-preserving. The rewrite from mutate-in-place to resolve-then-check is equivalent on every path I traced:
- explicit caller key present → returned first, env never consulted (precedence preserved, test-pinned)
- no caller key, trusted env key → used
- neither → same error message, same throw site
The apiKey = resolvedApiKey reassignment after the guard keeps the downstream createClient body untouched, so the diff surface stays honest. Error string is byte-identical, so nothing keying on it breaks.
Note on $env retention. The file still imports and uses $env for AZURE_OPENAI_API_VERSION. That is correct and deliberate — an API version string is not credential-bearing and not endpoint-deciding, so narrowing it would be unjustified scope creep. I checked specifically that no other credential- or endpoint-bearing read in this file was left on $env at this head; with #3268 applied, none are.
Test coverage. Five child-process tests over a controlled cwd: no key resolves to undefined, planted project key ignored, inherited key honored, project cannot override inherited, explicit caller key wins. That is the complete matrix for this resolver.
No duplicate/superseded overlap. Shares azure-openai-responses.ts with #3268 but at non-overlapping anchors (#3268: resolveAzureConfig, endpoint; #3272: createClient, credential). git merge-tree across both heads = 0 conflicts. Complementary halves of one boundary.
No blocking defects found.
Disposition
dev is currently red, so this is not merged despite being merge-ready. Recorded as a durable non-resident hold; merge once dev is green and the head is re-verified.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
2f9c9e1 to
74f2947
Compare
Credential residual named in the Yeachan-Heo#3263 review. createClient() fell back to $env.AZURE_OPENAI_API_KEY when the caller passed no key. $env merges the caller's cwd/.env, so repository content could supply the credential the client authenticates with. The codebase reserves $credentialEnv for exactly this case: "provider credential resolution must not use this merged view because it includes the caller's cwd/.env". Resolve the fallback through $credentialEnv. An explicit caller-supplied key still wins, and the launching shell plus GJC/user-owned .env files keep working; only repository-supplied values are rejected. The missing-key error text is unchanged. Regression: a child process with a controlled cwd (projectEnv is parsed at module load) proves a planted .env cannot supply the key, an inherited key still applies, a planted .env cannot override an inherited one, and an explicit caller key stays ahead of the environment. Reverting the resolver fails exactly the planted-.env case.
74f2947 to
0b13893
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — exact head 0b13893313666de153f1d8d254caf10d2df0422c.
Credential-boundary revalidation:
- The Azure client fallback now resolves
AZURE_OPENAI_API_KEYthrough$credentialEnv; an explicit caller key still wins and the missing-key error remains unchanged. - The semantic conflict with the already-merged endpoint-trust change was resolved by retaining both independent protections:
resolveAzureConfigForTestand trusted Azure endpoint resolution remain intact, while the key resolver is added immediately beside the client constructor. - The trust test covers no key, hostile project
.env, inherited operator key, project inability to override inherited input, and explicit caller precedence. The rebased test additionally neutralizes HOME and the agent directory so developer-owned trusted env files cannot change the no-key expectation or mask project-env rejection. - The head lands directly on current
devparent52a08fee19a3bb13be517cba195d6d04ccd82636; the release note was moved from released0.11.11toUnreleased. - Local focused suite: 5/5 passed. AI typecheck, Biome, and diff checks passed.
- Exact-head REST check-run evidence is terminal: 16 SUCCESS, 6 SKIPPED, 0 pending/failing. REST mergeability is
true/clean, with exact head/base re-pinned immediately before this verdict.
No credential material was printed or persisted during review.
— GJC security/trust review, 2026-07-28
Credential residual named in the #3263 review: “
createClientstill falls back to$env.AZURE_OPENAI_API_KEY”. Small and isolated on purpose.Problem
Bun.env === process.env, and the env module folds the caller'scwd/.envinto it, so a repository could supply the credential this client authenticates with. The codebase already states the rule this breaks:Honest severity
With the endpoint now bound to trusted sources (#3268 for
AZURE_OPENAI_BASE_URL/AZURE_OPENAI_RESOURCE_NAME), this is no longer a full redirect-plus-credential chain — it is a repository-controlled credential source that the project's own doctrine forbids. Fixing it removes the last way a repo can decide what the Azure client authenticates with.Fix
The fallback is lifted into a small resolver that uses
$credentialEnv. An explicit caller-supplied key still takes precedence; the launching shell,~/.env, the GJC config.envand the agent.envkeep working. The missing-key error text is unchanged.Tests
azure-api-key-trust.test.tsspawns a probe with a controlled cwd (the env module parsesprojectEnvat load time, so the boundary is only observable across processes): nothing supplies a key, a planted.envcannot supply one, an inherited key still applies, a planted.envcannot override an inherited one, and an explicit caller key stays ahead of the environment.Proof-first: reverting the resolver fails exactly
ignores an AZURE_OPENAI_API_KEY planted by the project .env.bun run checkinpackages/ai(whole-package biome + tsc): exit 0.Still open from that review
image-gen.ts—$env.OPENAI_BASE_URLat :1109 and$env.GOOGLE_API_KEYat :830/:853/:877 — is coming as its own PR so the two files stay independently reviewable.