Skip to content

fix(ai): resolve the Azure API key fallback from trusted env - #3272

Merged
Yeachan-Heo merged 1 commit into
Yeachan-Heo:devfrom
10kH:fix/azure-api-key-trust
Jul 29, 2026
Merged

fix(ai): resolve the Azure API key fallback from trusted env#3272
Yeachan-Heo merged 1 commit into
Yeachan-Heo:devfrom
10kH:fix/azure-api-key-trust

Conversation

@10kH

@10kH 10kH commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Credential residual named in the #3263 review: createClient still falls back to $env.AZURE_OPENAI_API_KEY. Small and isolated on purpose.

Problem

// azure-openai-responses.ts:264
if (!apiKey) {
  const envKey = $env.AZURE_OPENAI_API_KEY;   // merged view, includes cwd/.env
  ...
}

Bun.env === process.env, and the env module folds the caller's cwd/.env into it, so a repository could supply the credential this client authenticates with. The codebase already states the rule this breaks:

Provider credential resolution must not use this merged view because it includes the caller's cwd/.env. Use $credentialEnv/$pickCredentialEnv for model authentication.

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 .env and the agent .env keep working. The missing-key error text is unchanged.

Tests

azure-api-key-trust.test.ts spawns a probe with a controlled cwd (the env module parses projectEnv at load time, so the boundary is only observable across processes): nothing supplies a key, a planted .env cannot supply one, an inherited key still applies, a planted .env cannot 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 check in packages/ai (whole-package biome + tsc): exit 0.

Still open from that review

image-gen.ts$env.OPENAI_BASE_URL at :1109 and $env.GOOGLE_API_KEY at :830/:853/:877 — is coming as its own PR so the two files stay independently reviewable.

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
  • createClient uses that resolver and throws the same missing-key error when neither source supplies a key
  • Narrow resolveAzureClientApiKeyForTest seam 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:

  1. nothing supplies a key → null
  2. planted project .env key ignored
  3. inherited key honored
  4. planted cannot override inherited
  5. 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)

  1. 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.
  2. AZURE_OPENAI_API_VERSION / AZURE_OPENAI_DEPLOYMENT_NAME_MAP still $env — not credential material; same defense-in-depth note as on #3268.
  3. image-gen.ts residuals — out of file set; author correctly deferred.
  4. Whitespace-only caller apiKey remains 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-tree of #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 $credentialEnv endpoint resolution and this PR's $credentialEnv key 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 Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stale exact-head review refresh: re-affirming APPROVED at current head (no diff since last review).

@10kH
10kH force-pushed the fix/azure-api-key-trust branch from 328352d to 2f9c9e1 Compare July 27, 2026 13:43

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-merged dev commits 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-base confirms this PR carries the same single commit both before and after the rebase; every additional commit between the two heads is an independently-merged dev commit (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 Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-trust5 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) 🦞]

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.
@Yeachan-Heo
Yeachan-Heo force-pushed the fix/azure-api-key-trust branch from 74f2947 to 0b13893 Compare July 29, 2026 01:21

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE — exact head 0b13893313666de153f1d8d254caf10d2df0422c.

Credential-boundary revalidation:

  • The Azure client fallback now resolves AZURE_OPENAI_API_KEY through $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: resolveAzureConfigForTest and 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 dev parent 52a08fee19a3bb13be517cba195d6d04ccd82636; the release note was moved from released 0.11.11 to Unreleased.
  • 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

@Yeachan-Heo
Yeachan-Heo merged commit 0b10968 into Yeachan-Heo:dev Jul 29, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants