fix(ai,agent): resolve OpenAI/Azure endpoints from trusted env only - #3268
Conversation
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Hostile exact-head review — PR #3268
Terminal verdict: APPROVE / MERGE_READY
| Gate | Result |
|---|---|
| Exact head | 712014b7ed5a39196d9120834b085c0ea2e019b7 (matches requested; gh headRefOid == local refs/review/pr-3268) |
| 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 — 24 check-runs on this SHA: 19 success / 0 failure / 0 pending / 5 skipped. Includes test:packages/ai/test/openai-baseurl-trust.test.ts, test:packages/agent/test/compaction-endpoint-trust.test.ts, check:@gajae-code/ai, check:@gajae-code/agent-core, cli-smoke, native-build, gjc-state-gates, evidence producer. Dev CI run 30245055535 |
| Contributor surface | external fork 10kH:fix/openai-azure-baseurl-trust-v2 (cross-repo) |
What this head claims vs what the diff proves
Successor to #3263. Both signed blockers from that review are closed on this head:
Blocker 1 — Azure resource-name alternate constructor
resolveAzureConfig now resolves both AZURE_OPENAI_BASE_URL and AZURE_OPENAI_RESOURCE_NAME through $credentialEnv (not $env). $credentialEnv excludes project-only cwd/.env values (and ambiguous Bun-preloaded project matches) while still accepting inherited shell + GJC/user-owned env files.
Trust suite plants AZURE_OPENAI_RESOURCE_NAME=attacker-owned-resource and asserts the Azure base resolves to null (throw → probe null), plus inherited corp-resource → https://corp-resource.openai.azure.com/openai/v1 and planted cannot override inherited. That is the proof the prior suite lacked.
Blocker 2 — agent compaction half is now process-isolated
packages/agent/src/compaction/openai.ts swaps $env.OPENAI_BASE_URL → $credentialEnv("OPENAI_BASE_URL") with a package-local probe + compaction-endpoint-trust.test.ts (hosted default / plant ignored / inherited honored / plant cannot override). Revert-of-agent-only would fail the plant case. Package boundary preserved.
OpenAI provider sites
openai-responses/openai-completionsresolvers:$inheritedEnv ?? $env→$credentialEnvopenai-compatmodel-manager: same viaresolveOpenAIModelManagerBaseUrl- Narrow
*ForTestseams only; no production behavior change beyond the trust boundary
Hostile residual scan (non-blocking for this PR's contract)
- Azure
createClientstill falls back to$env.AZURE_OPENAI_API_KEY— still true on this head. Primary stream path already usesgetEnvApiKey→$credentialEnv; this is the residual #3272 isolates. Out of this PR's stated scope. model-registry.tsstill$pickenvfor provider base URLs — can still bake project.envintomodel.baseUrlbefore provider resolvers; re-admits redirect when configured base is non-default. Tracked as #3270. Out of file set.image-gen.tsstill$env.OPENAI_BASE_URL— out of file set; remaining authenticated-traffic residual class.AZURE_OPENAI_API_VERSIONandAZURE_OPENAI_DEPLOYMENT_NAME_MAPstill$env— not host constructors; deployment map can only retarget deployment id on an already-trusted Azure host. Defense-in-depth follow-up, not credential-exfiltration-via-base-URL.- Coverage nit: "project cannot override inherited OPENAI_BASE_URL" asserts
responses+modelManagerbut notcompletions— same resolver family; not a security hole.
Cross-PR order note (does not block APPROVE)
Independent contracts:
- #3270 is disjoint paths (coding-agent model-registry only) — clean vs this head.
- #3272 also edits
packages/ai/src/providers/azure-openai-responses.ts— textual conflict with this head if both merge without rebase (git merge-tree reports 2 "changed in both" regions). Prefer merge #3268 before #3272, or rebase #3272 onto post-#3268dev. Not a defect in this PR's delivery.
Verdict
APPROVE / MERGE_READY at exact head 712014b7ed5a39196d9120834b085c0ea2e019b7.
Both #3263 blockers are fixed with proof-first process-isolation tests. No security regression introduced in-scope. Residuals correctly left to #3270 / #3272 / image-gen. Any new head SHA voids this review.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
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).
712014b to
70ff715
Compare
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — exact head 70ff715e6974fdbe09ef629d401a4f27d2777c18
Rebase-only refresh of the prior exact-head review at 712014b7ed5a39196d9120834b085c0ea2e019b7 (APPROVE).
Checks performed:
- Content identity confirmed, not assumed:
diff <(git show 712014b7 --format= -- packages/agent/src/compaction/openai.ts packages/agent/test/compaction-endpoint-trust.test.ts packages/agent/test/fixtures/compaction-endpoint-probe.ts packages/ai/src/provider-models/openai-compat.ts packages/ai/src/providers/azure-openai-responses.ts packages/ai/src/providers/openai-completions.ts packages/ai/src/providers/openai-responses.ts packages/ai/test/fixtures/openai-baseurl-probe.ts packages/ai/test/openai-baseurl-trust.test.ts packages/ai/test/openai-model-discovery-env.test.ts) <(git show 70ff715e --format= -- packages/agent/src/compaction/openai.ts packages/agent/test/compaction-endpoint-trust.test.ts packages/agent/test/fixtures/compaction-endpoint-probe.ts packages/ai/src/provider-models/openai-compat.ts packages/ai/src/providers/azure-openai-responses.ts packages/ai/src/providers/openai-completions.ts packages/ai/src/providers/openai-responses.ts packages/ai/test/fixtures/openai-baseurl-probe.ts packages/ai/test/openai-baseurl-trust.test.ts packages/ai/test/openai-model-discovery-env.test.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: 19 success / 0 failure / 0 pending / 5 skipped (total 24). 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
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Adversarial exact-head review — PR #3268
Terminal verdict: MERGE_READY (held — see disposition)
| Gate | Result |
|---|---|
| Exact head | 70ff715e6974fdbe09ef629d401a4f27d2777c18 |
| Base | dev @ b853f15d7167a49173bc03ef7efe99c5bae8f234 |
| Mergeability | MERGEABLE / CLEAN; git merge-tree vs origin/dev = 0 conflicts |
| Exact-head CI | green — 19 pass / 0 fail / 5 skipped |
| Local focused | openai-baseurl-trust + compaction-endpoint-trust + openai-model-discovery-env → 15 pass / 0 fail |
| Local aggregate | check:@gajae-code/ai and check:@gajae-code/agent-core (biome + tsc --noEmit) clean |
What the change actually does
Routes four OpenAI/Azure endpoint decisions off the merged $env view and onto $credentialEnv:
packages/ai/src/providers/openai-responses.ts—resolveOpenAIProviderBaseUrlpackages/ai/src/providers/openai-completions.ts— same resolver, completions pathpackages/ai/src/provider-models/openai-compat.ts— model-manager base URLpackages/ai/src/providers/azure-openai-responses.ts—AZURE_OPENAI_BASE_URLandAZURE_OPENAI_RESOURCE_NAMEpackages/agent/src/compaction/openai.ts— remote-compaction endpoint
Adversarial findings
The stated vulnerability is real and correctly characterised. $env is Bun.env after the module-load merge of process.cwd()/.env (packages/utils/src/env.ts:77-83), so before this change a repository could plant a .env and redirect requests that carry the OpenAI/Azure credential. The pre-existing $inheritedEnv(...) ?? $env.OPENAI_BASE_URL?.trim() shape was strictly worse than useless: the ?? fallback re-admitted exactly the project value the first term was there to exclude. Collapsing to $credentialEnv is the correct fix, not a cosmetic one.
AZURE_OPENAI_RESOURCE_NAME inclusion is the sharpest catch here. It is the alternate constructor for the same host (https://<resource>.openai.azure.com/openai/v1); trust-boundarying only AZURE_OPENAI_BASE_URL would have left a fully equivalent bypass. Covered by a dedicated test.
No behavioral regression for legitimate configuration. $credentialEnv still resolves launching-shell values, ~/.gjc/agent/.env, ~/.gjc/.env, ~/.env, and home shell rc files — only cwd/.env is dropped. The suite pins both directions: planted project values ignored, inherited values still honored, and project values unable to override an inherited one.
Test design is genuinely hermetic. projectEnv is parsed at module load from process.cwd(), so in-process mutation cannot exercise this boundary; the tests correctly spawn a child with a controlled cwd via a probe fixture, and scrub the keys from the inherited child env first. The *ForTest seams are thin pass-throughs that add no production branching.
No duplicate/superseded overlap. #3270 (registry layer) and #3272 (Azure API key) touch disjoint call sites; #3268 and #3272 both edit azure-openai-responses.ts but at non-overlapping anchors — git merge-tree across the two heads yields 0 conflicts. They are complementary, not competing.
Non-blocking observation: packages/coding-agent/src/tools/image-gen.ts:1109 still reads $env.OPENAI_BASE_URL for the same class of decision. Out of scope here and already addressed by #3309.
Disposition
Verification note: two local failures initially appeared for the hosted-default assertions. Those were traced to this reviewer's own host ~/.zshrc / ~/.bashrc exporting OPENAI_BASE_URL — a value $credentialEnv is supposed to honor. Re-run under an isolated HOME: 15 pass / 0 fail. Not a defect in this PR.
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) 🦞]
Bun.env is process.env and the env module merges the caller's cwd/.env into it, so repository content could set where authenticated OpenAI/Azure requests go: openai-responses.ts:162 $inheritedEnv(OPENAI_BASE_URL) ?? $env.OPENAI_BASE_URL openai-completions.ts:105 same openai-compat.ts:561 $inheritedEnv(...) || $env.OPENAI_BASE_URL compaction/openai.ts:84 $env.OPENAI_BASE_URL azure-openai-responses.ts:237 $env.AZURE_OPENAI_BASE_URL The two provider paths already reached for $inheritedEnv, but the `?? $env` fallback re-admitted the project .env, so the guard only held when the variable was also present in the inherited environment. The other three had no guard. Resolve all five through $credentialEnv, the resolver this codebase already uses for material that must not come from the caller's project .env. It starts with the inherited environment and adds the GJC/user-owned .env files, so shell and user-level configuration keeps working; only repository-supplied values are rejected. Regression: a child process with a controlled cwd (projectEnv is parsed at module load) proves a planted .env cannot set either base URL, inherited values still apply, and a planted .env cannot override an inherited one. Reverting only openai-responses.ts (compiling cleanly) fails exactly the planted-.env case. Adds narrow ForTest seams for the module-private resolvers so the boundary is observable without exercising a live provider.
Successor to Yeachan-Heo#3263, addressing both signed blockers. Bun.env is process.env and the env module merges the caller's cwd/.env into it, so repository content could set where authenticated requests go: openai-responses.ts:162 $inheritedEnv(OPENAI_BASE_URL) ?? $env.OPENAI_BASE_URL openai-completions.ts:105 same openai-compat.ts:561 $inheritedEnv(...) || $env.OPENAI_BASE_URL compaction/openai.ts:84 $env.OPENAI_BASE_URL azure-openai-responses.ts $env.AZURE_OPENAI_BASE_URL and $env.AZURE_OPENAI_RESOURCE_NAME The first three already reached for $inheritedEnv, but the ?? / || fallback re-admitted the merged view, so the guard only held when the variable was also present in the inherited environment. The rest had no guard. Blocker 1: AZURE_OPENAI_RESOURCE_NAME is the alternate constructor for the same Azure host (https://<resource>.openai.azure.com/openai/v1), so leaving it on $env kept the endpoint redirectable after the base-URL fix. It now resolves through $credentialEnv, and the suite plants it as attacker input instead of only scrubbing it. Blocker 2: the agent half is now proven. packages/agent gets its own child process probe and trust suite, so reverting only compaction/openai.ts fails a planted-.env case. Revert proofs on this head: reverting only the Azure resource name fails exactly "ignores an AZURE_OPENAI_RESOURCE_NAME planted by the project .env"; reverting only the agent resolver fails exactly "ignores an OPENAI_BASE_URL planted by the project .env". Also renames an openai-model-discovery-env case whose title still described the removed $env fallback. Scope deliberately excludes the coding-agent residuals the review listed (image-gen, model-registry provider override, the Azure createClient key fallback).
70ff715 to
42d3570
Compare
2 participants
Successor to #3263 on a fresh head, addressing both signed blockers.
Blocker 1 — Azure endpoint still redirectable via
AZURE_OPENAI_RESOURCE_NAMEYou were right: fixing
AZURE_OPENAI_BASE_URLalone left the alternate constructor open.resolveAzureConfigfalls back tobuildDefaultBaseUrl(resourceName)→https://<resource>.openai.azure.com/openai/v1, so a plantedAZURE_OPENAI_RESOURCE_NAME=attacker-owned-resourcestill built the authenticated request host while the key kept arriving from the trusted path. My suite scrubbed that key from the outer env but never planted it, so CI could not catch it.AZURE_OPENAI_RESOURCE_NAMEnow resolves through$credentialEnv, and the suite plants it as attacker input. Added cases: planted resource name ignored, inherited resource name still buildshttps://corp-resource.openai.azure.com/openai/v1, and a planted.envcannot override an inherited resource name.Revert proof: reverting only that one line fails exactly
ignores an AZURE_OPENAI_RESOURCE_NAME planted by the project .env(8 pass / 1 fail).Blocker 2 — agent half is now proven
packages/agentgets its own child-process probe (test/fixtures/compaction-endpoint-probe.ts) and trust suite (test/compaction-endpoint-trust.test.ts), keeping the package boundary rather than reaching across frompackages/ai. Cases: hosted default, planted.envignored, inherited base URL honored, planted.envcannot override an inherited one.Revert proof: reverting only
packages/agent/src/compaction/openai.tsfails exactlyignores an OPENAI_BASE_URL planted by the project .env(3 pass / 1 fail).Unchanged from #3263
The five OpenAI/Azure sites resolve through
$credentialEnv, which begins with$inheritedEnvand adds the GJC/user-owned.envfiles — shell and user-level configuration keep working, only repository-supplied values are rejected. Narrow*ForTestseams follow the existing package convention.Residuals
Per your instruction I did not expand scope:
image-gen.ts,model-registry.ts#resolveProviderOverride, and the AzurecreateClient$env.AZURE_OPENAI_API_KEYfallback are untouched and remain follow-ups. I did fix the one drift my own change caused — anopenai-model-discovery-envcase whose title still described the removed$envfallback.Verification
bun run check(whole-package biome + tsc):packages/aiexit 0,packages/agentexit 0.openai-baseurl-trust: 9 pass / 0 fail.compaction-endpoint-trust: 4 pass / 0 fail.packages/agent/test: 598 pass / 0 fail.