Uh oh!
There was an error while loading. Please reload this page.
test(e2e): C7 /passthrough/anthropic auth-shape switching + verbatim forward - #165
Merged
Conversation
…forward (#151) Third endpoint covered from #151's C7 row. Per gateway docs §4.10, /passthrough/{provider}/*rest is the lowest-overhead escape hatch for provider endpoints aisix doesn't natively wrap (OpenAI batches / files / fine-tuning, Anthropic message-batches, etc.). Prior to this file the gateway had **zero** e2e coverage on /passthrough — every customer using batches/files had no regression protection on the wire. Case shipped: Anthropic passthrough: caller POSTs /passthrough/anthropic/v1/messages/batches with arbitrary body. Gateway must: - Strip /passthrough/anthropic prefix and forward the rest (`/v1/messages/batches`) verbatim to upstream - Inject Anthropic's auth shape: `x-api-key` + `anthropic-version` headers (NOT `Authorization: Bearer` — a regression that forwarded Bearer to Anthropic upstream would 401 in production but pass against a permissive mock; the explicit auth header pin is the only line of defence) - Forward request body byte-for-byte (the entire point of passthrough is "we don't normalize the body") - Round-trip the upstream's response shape to the caller Case held back: OpenAI passthrough surfaced a documented contract conflict between docs/api-admin.md §4.3 (api_base example `https://api.openai.com/v1`) and docs/api-proxy.md §4.10 (passthrough example `/passthrough/openai/v1/batches`) — together they produce double-/v1 upstream URL. Filed as #164. Same hold-back pattern as #153 / #154 / #159 / #162. References: - Gateway's own /passthrough contract: `docs/api-proxy.md` §4.10 - Anthropic auth headers spec <https://docs.anthropic.com/en/api/getting-started> Refs #151
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. 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 (1)
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 |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end coverage for the gateway’s /passthrough/{provider}/*rest escape hatch, specifically pinning Anthropic passthrough behavior to prevent auth-shape and wire-format regressions.
Changes:
- Introduces a new e2e test covering
POST /passthrough/anthropic/v1/messages/batches. - Asserts passthrough path stripping (upstream sees
/v1/messages/batches) and verbatim request-body forwarding. - Asserts Anthropic-specific auth-shape switching (
x-api-key+ non-emptyanthropic-version) for upstream requests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(typeof testCalls[0]?.headers["anthropic-version"]).toBe("string"); | ||
| expect( | ||
| (testCalls[0]?.headers["anthropic-version"] as string).length, | ||
| ).toBeGreaterThan(0); |
Audit (per CLAUDE.md §8) found two MEDIUM: MEDIUM M1 (caller-bearer-leak assertion missing despite being one of the PR's stated motivations): Initial PR's assertions verified `x-api-key: sk-ant-mock` correct injection but did NOT pin that the caller's plaintext bearer was absent on the upstream side — a regression that injected x-api-key correctly AND forwarded the caller's Bearer would silently pass. Audit recommended `expect(authorization).toBeUndefined()`. That assertion FAILED (gateway today sends `Authorization: Bearer sk-ant-mock` redundantly alongside x-api-key) — surfacing a separate issue: NOT a caller-bearer leak (the value is the gateway's own provider secret, not the caller's plaintext), but a wire-cleanliness gap vs docs §4.10 which implies x-api-key + anthropic-version only for Anthropic. Filed separately as #166. Settled assertion: pin the LEAK concern explicitly — if the upstream Authorization is present, its value must NOT contain the caller's plaintext bearer. Catches the regression mode the PR description claimed to defend against. MEDIUM M2 (anthropic-version pinned only as "non-empty string"): Pinned exact value `"2023-06-01"` per Anthropic's documented current API version (<https://docs.anthropic.com/en/api/getting-started>). A regression that injected a malformed-but-non-empty value (e.g. "v1", "latest", "unspecified") would 400 against real Anthropic but pass against the permissive mock without this exact pin. MEDIUM M3-M4 (HTTP method matrix, provider matrix gaps): Acknowledged scope deferrals; not addressed in this PR. Worth filing as separate follow-up. LOW L1-L3: not addressed. All 33 e2e tests still pass locally.
Closed
4 tasks
Uh oh!
There was an error while loading. Please reload this page.
3 tasks
moonming added a commit
that referenced
this pull request
May 9, 2026
Closes#157 (test-infra concern, not a product bug). Issue: the e2e suite's `guardrail-keyword-e2e.test.ts` has flaked three times on CI in the past 24 hours (#157 first occurrence, plus reruns required on PR #165 and #167). Failure mode is the same: `waitConfigPropagation: condition not met within 5s`. Root cause: vitest is configured with `maxForks: 4` (per `vitest.config.ts`), so up to 4 test files run in parallel, each spawning its own `aisix` binary against a SHARED etcd. Each binary opens watches and writes resources via the admin API concurrently with the others. Under that load, etcd watch dispatch latency for the LAST resource in a multi-resource batch (e.g. a Guardrail rule following Model + ApiKey + ProviderKey writes) can exceed the 5s budget. The 5s budget was sized when the suite had ~9 files. The suite is now 20+ files (#161, #163, #165, #167 added embeddings, responses, passthrough, rerank, images). The growth in parallelism load wasn't matched by a budget bump. Fix: raise the deadline to 10s. This: - Eliminates the recurring rerun churn on feature PRs - Preserves the "fail loudly on a genuinely stuck snapshot" property — 10s is still a generous floor; a real bug where propagation hangs indefinitely would still fail clearly - Doesn't change the happy-path latency at all (the helper polls every 50ms and returns as soon as the condition is met — bumping the deadline only affects the sad path) This is a test-infra-only change; no product behavior is affected. The ≤500ms spec target for in-process propagation is unchanged; this is purely the CI test harness's wait budget for slow runners under concurrent load. If 10s proves insufficient as the suite grows further, the next escalation would be to reduce `maxForks` from 4 to 2 (slower wall time, less etcd pressure) — tracked in #157 as a fallback.
moonming added a commit
that referenced
this pull request
May 9, 2026
… (#169) * test(harness): raise waitConfigPropagation budget 5s → 10s (#157) Closes#157 (test-infra concern, not a product bug). Issue: the e2e suite's `guardrail-keyword-e2e.test.ts` has flaked three times on CI in the past 24 hours (#157 first occurrence, plus reruns required on PR #165 and #167). Failure mode is the same: `waitConfigPropagation: condition not met within 5s`. Root cause: vitest is configured with `maxForks: 4` (per `vitest.config.ts`), so up to 4 test files run in parallel, each spawning its own `aisix` binary against a SHARED etcd. Each binary opens watches and writes resources via the admin API concurrently with the others. Under that load, etcd watch dispatch latency for the LAST resource in a multi-resource batch (e.g. a Guardrail rule following Model + ApiKey + ProviderKey writes) can exceed the 5s budget. The 5s budget was sized when the suite had ~9 files. The suite is now 20+ files (#161, #163, #165, #167 added embeddings, responses, passthrough, rerank, images). The growth in parallelism load wasn't matched by a budget bump. Fix: raise the deadline to 10s. This: - Eliminates the recurring rerun churn on feature PRs - Preserves the "fail loudly on a genuinely stuck snapshot" property — 10s is still a generous floor; a real bug where propagation hangs indefinitely would still fail clearly - Doesn't change the happy-path latency at all (the helper polls every 50ms and returns as soon as the condition is met — bumping the deadline only affects the sad path) This is a test-infra-only change; no product behavior is affected. The ≤500ms spec target for in-process propagation is unchanged; this is purely the CI test harness's wait budget for slow runners under concurrent load. If 10s proves insufficient as the suite grows further, the next escalation would be to reduce `maxForks` from 4 to 2 (slower wall time, less etcd pressure) — tracked in #157 as a fallback. * test(harness): also reduce maxForks 4→2 (#157 fallback escalation) After the timeout bump landed, #169's own CI run still flaked with the same `condition not met within 10s` error — meaning etcd watch dispatch latency genuinely exceeds 10s under maxForks=4 with the current suite size (20+ files). The audit on #169 had flagged this as the "product-side hypothesis still open" — confirmed. Apply the documented fallback from #157: cut maxForks from 4 to 2. This halves the concurrent-watcher count against the shared etcd and brings dispatch latency back inside the budget. Trade-off: wall time grows ~1.5-2× per CI run (locally measured ~14s @ maxForks=4 vs ~30s @ maxForks=2, so faster than expected because the extra parallelism wasn't fully utilized anyway — contention dominated). Net for CI is "predictable green" vs "fast but constantly-rerunning". The 10s `waitConfigPropagation` budget from the prior commit stays in place as belt-and-suspenders — even with maxForks=2 the original 5s would still be tight on slow runners. Combined this PR now does: 1. waitConfigPropagation deadline 5s → 10s (`harness/admin.ts`) 2. maxForks 4 → 2 (`vitest.config.ts`) The product-side hypothesis ("etcd watch dispatch degrades non-linearly with concurrent watchers") is now strongly supported and worth a separate product-side investigation tracked in #157 follow-up.
moonming added a commit
that referenced
this pull request
May 10, 2026
#166) (#209) ## Problem Per `docs/api-proxy.md` §4.10, Anthropic passthrough was supposed to inject the documented Anthropic auth shape — `x-api-key` + `anthropic-version` — and only that. In practice the gateway was ALSO injecting `Authorization: Bearer <provider-secret>` alongside: ``` x-api-key: sk-ant-mock ← correct per Anthropic spec anthropic-version: 2023-06-01 ← correct per Anthropic spec Authorization: Bearer sk-ant-mock ← redundant, not in Anthropic's auth contract ``` Real Anthropic ignores the redundant Bearer today, but: - Future stricter Anthropic gateways or customer-side middleware could reject the request based on having unexpected auth headers. - Operators inspecting upstream traffic captures generate "is this a leak?" tickets every time the redundant header surfaces — even though the value is the gateway's own provider secret, not the caller's plaintext bearer. Not a credential leak (verified by #165's existing assertion that the caller's plaintext bearer never reaches the upstream), but a docs-vs-behavior gap that operators shouldn't have to investigate. ## Fix In `crates/aisix-proxy/src/passthrough.rs::dispatch`, restructure the auth-injection branch so each provider gets EXACTLY ONE auth shape: - openai / gemini / deepseek / unknown providers → `Authorization: Bearer …` - anthropic → `x-api-key` + `anthropic-version` ONLY - empty `api_key` → no auth headers at all (was previously: still injected an empty `x-api-key` for anthropic — also cleaned up) ```rust if api_key.is_empty() { // nothing to inject } else if provider_lower == "anthropic" { builder = builder.header("x-api-key", &api_key); builder = builder.header("anthropic-version", "2023-06-01"); } else { builder = builder.header(header::AUTHORIZATION, format!("Bearer {api_key}")); } ``` ## Tests **Rust unit (`crates/aisix-proxy/src/passthrough.rs`)** — new `anthropic_passthrough_does_not_inject_bearer_auth` test: - Configures Anthropic passthrough with secret `sk-ant-test` - Posts to `/passthrough/anthropic/v1/messages/batches` - wiremock subset-match on `x-api-key` + `anthropic-version` - **Strict assertion**: drains `received_requests` and asserts `!headers.contains_key("authorization")` — the redundant Bearer must NOT be on the wire - Sanity: positive value-pin on `x-api-key: sk-ant-test` and `anthropic-version: 2023-06-01` **E2E (`tests/e2e/src/cases/passthrough-e2e.test.ts`)** — tightened the existing Anthropic case from the lenient `if (auth !== undefined) expect(auth).not.toContain(CALLER_PLAINTEXT)` to the strict `expect(headers["authorization"]).toBeUndefined()`. The lenient version caught caller-plaintext leaks (covered by #165); the strict version catches the wire-cleanliness regression #166 fixes. Helpers added: `anthropic_model()` and `anthropic_provider_key_entry()` for the Anthropic-shaped fixtures (parallel to the existing `openai_model` / `provider_key_entry`). ## Verification - `cargo test -p aisix-proxy --lib`: **152/152 passing** (incl. new test) - `cargo clippy -p aisix-proxy --lib --tests -- -D warnings`: clean - `cargo fmt --check`: clean - `pnpm tsc --noEmit` (e2e): clean ## References - Issue: #166 - `docs/api-proxy.md` §4.10 (passthrough contract) - Anthropic auth spec: <https://docs.anthropic.com/en/api/getting-started>
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
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
Third endpoint covered from #151's C7 row. Per gateway docs §4.10, `/passthrough/{provider}/*rest` is the lowest-overhead escape hatch for provider endpoints aisix doesn't natively wrap (OpenAI batches / files / fine-tuning, Anthropic message-batches). Prior to this PR the gateway had zero e2e coverage on `/passthrough`.
One of two originally-planned cases ships here. The other surfaced a docs/behavior conflict and is held back as #164.
What's pinned
Held back as separate issues
🐛 #164 — `/passthrough/openai/*` doubles `/v1` prefix when api_base ends with `/v1` (contradicts published docs). `docs/api-admin.md` §4.3 publishes `api_base: "https://api.openai.com/v1"\`; `docs/api-proxy.md` §4.10 publishes the call as `/passthrough/openai/v1/batches`. Together: `/v1/v1/batches` upstream. OpenAI passthrough test will return once reconciled.
🐛 #166 — `/passthrough/anthropic` injects redundant `Authorization: Bearer` alongside x-api-key. NOT a credential leak (Bearer value is gateway's own provider secret, not caller's plaintext) but a wire-cleanliness gap vs docs §4.10 implication of x-api-key + anthropic-version only for Anthropic. Test pins the leak concern explicitly; the strict "Authorization absent" assertion is the verification once #166 is resolved.
Why this matters
Three real-world failure modes that prior coverage couldn't catch:
Authorization: Bearer <provider-secret>alongside x-api-key #166's wire-cleanliness gap), it does NOT contain the caller's plaintext bearer.Source-blind discipline
Every assertion derives from external contracts:
No internal Rust paths or struct field names referenced.
Independent audit
Per CLAUDE.md §8, an independent audit agent reviewed the initial commit (4ffd636). Resolution log:
The audit's recommended strict assertion (`Authorization === undefined`) revealed a separate gateway-side issue (#166): NOT a caller-bearer leak, but a redundant Bearer injection alongside x-api-key. The shipped assertion pins the leak concern explicitly while #166 tracks the wire-cleanliness gap.
Test plan
Refs #151.