Uh oh!
There was an error while loading. Please reload this page.
fix(platform-adapters): honor ProviderKey.api_base in Vertex + Azure bridges (#390, #391) - #392
Merged
Merged
Conversation
Pre-fix, `VertexBridge::resolve_api_base()` consulted `self.api_base_override` only under `#[cfg(test)]`. Production builds dropped the line entirely, hardcoding `https://<region>-aiplatform.googleapis.com`. A BYO operator who set `ProviderKey.api_base` to a corporate-proxy URL saw their traffic still go to Google — no error, no warning, no log. Mirrors the Bedrock pattern (`crates/aisix-provider-bedrock/src/bridge.rs:597-609`) which already reads `ctx.provider_key.api_base.as_deref()` in production builds. Precedence on the rewritten Vertex path: 1. `#[cfg(test)] Self::with_api_base_override` — test seam (keeps the 7 existing wiremock-based unit tests working without touching ProviderKey state). 2. `ProviderKey.api_base` — production override for corporate-proxy / private-VPC / mock deployments. Trimmed of whitespace; trailing `/` stripped so URL stitching produces a single-slash separator. 3. Canonical `https://<region>-aiplatform.googleapis.com`. Empty string and whitespace-only `api_base` values are treated as absent — falls through to canonical, matching the OpenAI bridge's defensive normalization at `bridge.rs:107`. ## Out of scope (called out in PR body / docs) - **Token-mint endpoint.** OAuth minting hits `secret.token_uri` (operator-controlled inside the SA JSON), NOT `api_base`. An air-gapped operator behind a proxy must additionally set `token_uri` in the SA JSON to their internal token endpoint. The fix does not (and intentionally is not changed to) override `token_uri` from `api_base`. Documented in `provider-keys.md` row for `google-vertex`. - **Suffix normalization.** OpenAI bridge's `strip_known_endpoint` tolerates `api_base="https://api.openai.com/v1/chat/completions"`. Vertex equivalent (operator pasting a full `:generateContent` URL) is not in scope — Vertex's URL is parameterized over project/region/model and the canonical form is the bare host root. - **Azure-OpenAI (ai-gateway#391).** Sibling fix; same shape; separate PR after #390 lands. The branch carries the WIP in a stash for that next step. - **Anthropic-on-Vertex.** `adapter=vertex` + Anthropic publisher (Phase E D5.3) lands later and will pick up the same override path for free. ## Tests Four new unit tests on `resolve_api_base`: - `resolve_api_base_honors_ctx_api_base_in_production_path` — the new override path. Pre-fix this would have returned the canonical URL. - `resolve_api_base_trims_trailing_slash_on_ctx_override` — defends against operator paste of `http://proxy/`. - `resolve_api_base_falls_back_to_canonical_on_empty_ctx_value` — empty, whitespace, and None all fall back to canonical. - `resolve_api_base_cfg_test_override_takes_precedence_over_ctx` — preserves the existing 7-test wiremock seam. Two new end-to-end wiremock tests (production-path coverage, NO `with_api_base_override`): - `chat_gemini_honors_provider_key_api_base_in_production_path` — ProviderKey.api_base = wiremock URI, asserts wiremock observes the request at the expected path. `expect(1)` would have failed pre-fix because traffic went to the canonical Google URL instead. - `chat_gemini_trims_trailing_slash_on_provider_key_api_base` — operator paste with trailing slash still produces single-slash URL after stitching. ## Unblocks AISIX-Cloud platform-adapter live-spec work — once a new `:dev` image with this fix is published, the per-adapter `adapter-vertex-live.spec.ts` (issue #390 mentions it as 'blocked') can be authored to assert the customer-visible contract end-to-end via the `mock-vertex` compose service. Refs: #390, AISIX-Cloud#492 (real-cloud Bedrock/Vertex/Azure smoke tests tracking).
Warning Review limit reached
Your plan includes 3 reviews of capacity. Refill in 8 minutes and 4 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughVertexBridge now resolves Vertex API base using test seam, ProviderKey.api_base (trimmed/stripped), or canonical per-region host; both Gemini paths wire the override. AzureOpenAI gains a verbatim upstream_override mode with validation and URL-building changes. Tests and docs updated accordingly. ChangesVertex API Base Override Support
Azure OpenAI upstream_override Support
🎯 4 (Complex) | ⏱️ ~45 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
…#391) Pre-fix, `AzureUpstreamRef::resolve()` rejected any `api_base` whose host did not end in `.openai.azure.com` with a `Config` error, blocking BYO deployments through corporate proxies, private-VPC endpoints, or mock services. The only override seam was `#[cfg(test)] pub(crate) with_url_override`, unreachable from production builds. This is the parity gap that surfaced from AISIX-Cloud#491 live e2e work — mirroring Bedrock, the operator-supplied `ProviderKey.api_base` should be honored as a production override. (Vertex got the same treatment in the previous commit, #390.) ## What changed `AzureUpstreamRef` gains an `upstream_override: Option<String>` field. `resolve()` keeps the canonical Azure path unchanged (a `.openai.azure.com`- suffixed `api_base` still extracts `resource` and uses the canonical URL builder), AND adds a verbatim-override branch for any other HTTP/HTTPS host: `resource` is empty, `upstream_override` holds the base, and `chat_completions_url()` builds the URL off the override instead of `<resource>.openai.azure.com`. Defence-in-depth on the override path: reject any `api_base` that contains `?` or `#`. The bridge's canonical query-string append (`?api-version=…`) would otherwise either merge with or be overridden by an operator-supplied query, opening an api-version downgrade vector. The bare-resource shorthand and the canonical `.openai.azure.com` path keep their existing strict validation. ## Tests - `resolve_accepts_verbatim_override_for_non_canonical_host` — pins the new contract (was `resolve_rejects_canonical_https_with_wrong_suffix`, renamed + flipped to match the new behavior, with a docstring citing #391). - `resolve_accepts_http_override_for_mock_endpoint` — canonical e2e shape (`http://mock-llm:8000` redirected to mock-llm's Azure deployment endpoint). - `resolve_strips_trailing_slash_on_verbatim_override` — ensures no `//` between the override base and the appended deployment path. - `resolve_rejects_override_with_query_injection` — explicit defence on the api-version downgrade vector. - `resolve_rejects_override_with_fragment` — defence on URL fragment injection. All pre-existing tests pass unchanged. ## References - Bedrock parity: `crates/aisix-provider-bedrock/src/bridge.rs` reads `ctx.provider_key.api_base.as_deref()` and forwards it as the AWS SDK's `endpoint_url`. - Companion fix: #390 (Vertex `resolve_api_base`). - Unblocks AISIX-Cloud follow-up: per-adapter `adapter-azure-live.spec.ts` exercising `dashboard → cp-api → DP → mock-llm Azure-shape` end-to-end.
… path preservation (#391) Independent audit findings from PR #392: H1 (Typo guard for near-miss canonical hosts) ↳ Documented explicitly in the `upstream_override` field doc-comment. The audit's suggested heuristic (`rest.contains("openai.azure.com") || rest.contains("openai.azure.")`) doesn't actually catch the cited typo example (`openai.azur.com` contains neither substring) and would break legitimate Azure cloud variants (`.openai.azure.cn` Azure China, `.openai.azure.us` Azure Government) plus any corporate proxy whose URL happens to contain `openai`. Same risk model as OpenAI / Bedrock bridges that already accept arbitrary operator-supplied `api_base`. Documented the typo caveat in the field doc so operators are warned at API surface time. M1 (Path-preservation contract for verbatim override) ↳ Added `resolve_preserves_path_prefix_on_verbatim_override` test. The bridge preserves an internal path prefix (e.g. corporate gateway routing `/azure-passthrough/*` to upstream Azure) and appends the deployment path verbatim. Test pins the shape. M2 (Public struct gained field — non_exhaustive) ↳ Added `#[non_exhaustive]` to `AzureUpstreamRef`. Future field additions no longer require a major version bump for downstream crates. In-crate test struct-literal at line 924 still works (`#[non_exhaustive]` only restricts external callers). Added docstring guidance pointing external callers at `Self::resolve`. Symmetry with Vertex sibling fix ↳ Added `@` userinfo rejection on the verbatim-override branch. An operator embedding `user:pass@host` would leak credentials via logs and bypass the api-key / AAD auth path. Mirrors the vertex-bridge defence at `resolve_api_base`. Test `resolve_rejects_override_with_userinfo` pins the rejection. All pre-existing tests pass; new tests: - `resolve_preserves_path_prefix_on_verbatim_override` - `resolve_rejects_override_with_userinfo` LOW findings (Vertex docstring "mirrors Bedrock" wording, query-injection asymmetry across canonical/override branches, `resource` field doc) are informational — not addressed in this commit; can land in a follow-up docs PR if they surface as real-world confusion.
…on (#390) Audit findings on PR #392 surfaced 1 HIGH + 1 MEDIUM that needed in-PR fixes: **HIGH-1 (streaming-path E2E coverage gap):** the original PR updated both `chat_gemini` and `chat_gemini_stream` call sites to thread `ctx.provider_key.api_base`, but the new wiremock test only exercised the non-streaming path. A regression that reverted just `chat_gemini_stream` would have compiled clean and passed all tests. Add `chat_gemini_stream_honors_provider_key_api_base_in_production_path` — mirrors the non-streaming version but drains the SSE stream via `bridge.chat_stream`. **MEDIUM-1 (api_base SSRF / credential-embed surface):** before this PR the production code path hardcoded the canonical Google URL — there was NO operator-supplied URL surface. The fix opened that surface; the original commit forgot to validate it. `api_base` is interpolated directly into `format!("{base}/v1/projects/...")` so malicious / mistaken input could: - use a non-http(s) scheme (`file:///etc/passwd`, `gs://...`) - embed userinfo (`https://user:pass@host`) → credential leak via access-log URLs + auth-header shadow concerns - inject a query string that silently merges with the bridge's own `?alt=sse` on the streaming path - inject a fragment Add validation that rejects all four classes as `BridgeError::Config` (matches Azure-OpenAI sibling fix#391's pattern of surfacing operator-actionable Config errors rather than silently falling through to canonical). `resolve_api_base` signature now returns `Result<String, BridgeError>`; both call sites propagate with `?`. Five new unit tests cover the four rejection classes + a happy-path sanity check. ## Test run - `cargo test -p aisix-provider-vertex` — 72/72 green (was 67/67 pre-audit, added 5 tests). - `cargo clippy -p aisix-provider-vertex --all-targets -- -D warnings` — clean (fixed one doc-lazy-continuation in the new docstring). - `cargo fmt -p aisix-provider-vertex` — clean. Refs: #390, PR #392 audit.
moonming
commented
May 25, 2026
MemberAuthor
Audit findings addressed in fe13041. HIGH-1 (streaming-path E2E gap): added MEDIUM-1 (api_base SSRF / credential-embed surface): before #390 the production code path hardcoded the canonical Google URL — there was no operator-supplied URL surface. The fix opened that surface; the original commit forgot to validate.
Mirrors the Azure-OpenAI sibling fix (#391) which rejects the same classes. Surfacing as Config errors (vs silently falling through to canonical) matches the principle of #390 itself: don't silently ignore operator config. Five new unit tests cover the four rejection classes:
LOW-1 and LOW-2 from the audit are non-blocking; no action. Test run: 72/72 green (added 5 tests, was 67/67 pre-audit). |
… LOW-1)
Re-audit LOW-1 noted that the userinfo-rejection error string echoes
the operator-supplied URL back verbatim via `{b:?}` — so a rejected
`api_base="https://user:secret@host"` produces a Config error
containing "user:secret@host". That string propagates to operator-
facing logs; rejecting `@` exists specifically to keep operator
credentials out of upstream traffic, so the error shouldn't leak
them via the audit/telemetry path either.
Add `redact_userinfo` helper that replaces `scheme://user:pass@host`
with `scheme://<redacted>@host`. Only touches the substring between
`://` and the first `@`; leaves `@` past the first path slash
intact (RFC 3986 allows `@` in path segments).
Tests:
- `redact_userinfo_strips_user_and_password` — happy path
- `redact_userinfo_leaves_non_userinfo_at_alone` — RFC 3986 path-`@`
is preserved (`https://proxy.internal/v1@my-namespace` unchanged)
- The existing userinfo-rejection test now asserts the rejected URL
contains `<redacted>` and does NOT contain the original
`user:secret` substring.
74/74 Vertex tests green (+2 redactor tests on top of the +5
SSRF-validation tests from the prior audit-fix commit).
`cargo clippy --all-targets -- -D warnings` clean.
Refs: #390, PR #392 re-audit LOW-1.moonming
commented
May 25, 2026
MemberAuthor
Re-audit LOW-1 addressed in 46f6997. The userinfo-rejection branch's error message echoed the operator-supplied URL verbatim via Added a small 74/74 Vertex tests green. The re-audit's LOW-2 note ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two parallel production gaps where
ProviderKey.api_basewas silently dropped or rejected by the bridge, fixed in lockstep so the Bedrock parity story now covers all three platform adapters.api_basevia SDKendpoint_url(bridge.rs:603)resolve_api_base()only consulted#[cfg(test)]api_base_override; production silently fell back to<region>-aiplatform.googleapis.comctx_api_basearg with precedence: cfg-test override >ctx.provider_key.api_base> canonicalAzureUpstreamRef::resolve()rejected anyapi_basewhose host didn't end in.openai.azure.comupstream_overridefield + verbatim-override branch for non-canonical hosts; canonical Azure path unchangedWhy now
Surfaced by AISIX-Cloud#491 — when writing per-adapter live e2e specs (
adapter-{azure,vertex,bedrock}-live.spec.ts) against compose-stack mocks, the Bedrock spec worked because the bridge honoredapi_base, but Vertex + Azure could not redirect tomock-vertex/mock-llm. Filed as ai-gateway#390 + #391; this PR closes both.Defence-in-depth
The Azure override branch explicitly rejects
?and#inapi_base— the bridge appends?api-version=…so an operator-supplied query would open an api-version downgrade vector. The canonical Azure path keeps its existing strict resource-name validation.The Vertex change drops nothing — the
#[cfg(test)] with_api_base_overrideseam is preserved and takes precedence overctx.provider_key.api_base, so existing tests keep working unchanged.Tests
Vertex (4 new unit tests in
bridge.rs::tests)resolve_api_base_honors_ctx_api_base_in_production_path— pins the new production override pathresolve_api_base_trims_trailing_slash_on_ctx_overrideresolve_api_base_falls_back_to_canonical_on_empty_ctx_value(empty string, whitespace, None)resolve_api_base_cfg_test_override_takes_precedence_over_ctx— proves wiremock-pinning tests stay intactAzure (5 unit-test changes in
bridge.rs::tests)resolve_accepts_verbatim_override_for_non_canonical_host— replaces (renamed + flipped) the pre-fixresolve_rejects_canonical_https_with_wrong_suffixtest, with an inline docstring citing Azure-OpenAI bridge hardcodes .openai.azure.com host suffix, blocks BYO api_base override #391resolve_accepts_http_override_for_mock_endpoint— pins the canonical mock-llm e2e shaperesolve_strips_trailing_slash_on_verbatim_overrideresolve_rejects_override_with_query_injectionresolve_rejects_override_with_fragmentAll pre-existing tests (canonical Azure path, bare-resource shorthand, validators) pass unchanged.
Test plan
cargo test --workspace— 1200+ tests pass, 0 failedcargo clippy -p aisix-provider-vertex -p aisix-provider-azure-openai --tests -- -D warnings— cleancargo fmt— appliedCloses
ProviderKey.api_base.openai.azure.comhost suffixUnblocks
adapter-vertex-live.spec.ts+adapter-azure-live.spec.ts(per-adapter live e2e specs, mirror of the mergedadapter-bedrock-live.spec.tsin AISIX-Cloud#491)Summary by CodeRabbit
Bug Fixes
New Features
Validation & Tests
Documentation