Repository navigation
fix(headers): complete the exact-name rule, and carry the forward on /v1/realtime - #1111
Merged
Merged
Conversation
…/v1/realtime
Four follow-ups to the one auth-independent client-header forward.
The exact-name rule for credential slots was implemented against a fixed
list of names. That list is complete on `/v1/*` and MCP, where the caller's
gateway credential always arrives in `Authorization` or `x-api-key`. It is
not complete on a passthrough route, where the operator names the slot:
under `auth_mode: header_key` the credential arrives in the route's
`auth_header_name`, and route validation guarantees that name is never one
of the fixed ones. `forward_client_headers: ["x-*"]` beside
`auth_header_name: x-gw-key` therefore relayed every caller's gateway key
to the upstream, where it can be replayed. `identity_header` had the same
shape, against a field whose description promises it is stripped before
forwarding.
A route's own `auth_header_name` and `identity_header` now join the
exact-name set for that route. A glob no longer reaches either; naming
one in full still forwards it, unchanged. No existing configuration
changes meaning except one that was relaying a credential it never named.
`/v1/realtime` never carried the setting at all: it builds its upstream
WebSocket handshake by hand rather than through the shared pipeline, so an
operator who declared that an upstream reads the caller's own credential
got it on every other `/v1/*` endpoint and not there. It now applies the
same forward with the same precedence, refusing the handshake slots this
surface owns — the caller's `sec-websocket-key` / `-version` /
`-extensions` describe the connection the caller opened, and the browser
flow puts the caller's own gateway key in `sec-websocket-protocol`.
`request.default_headers` remains unapplied on this face, as before.
The `ensemble` and `semantic` sub-dispatch paths were audited and already
correct: a panel member, a judge (buffered and streamed) and a semantic
route target each carry the caller's forwardable headers, resolved against
the ProviderKey of the upstream actually being called rather than the
entry's — the ensemble entry has no ProviderKey of its own. The semantic
router's embedding lookup deliberately forwards nothing: it is a gateway
classification call the caller never addressed. Covered by e2e so a future
sub-dispatch cannot drop it silently.
Finally, the `forward_client_headers` description said an AWS Bedrock
upstream refuses four names; the code refuses six. `x-amz-target` and
`x-amzn-bedrock-accept` were missing from the prose, and the reserved list
is now pinned by a test so the two cannot drift again.
Behavior changes since 0.13.0, both from the preceding change and stated
here because release notes are built from commits:
- `default_headers` filtering widened. 0.13.0 filtered the block only
against credential names, so gateway-namespace headers passed through;
the filter now also excludes the `x-aisix-` prefix. An existing
`default_headers: {"x-aisix-foo": "..."}` stops being delivered, and
because the control plane re-validates the merged block, that provider
key is rejected on edit until the header is removed. The error is
explicit and the configuration is dead on the new data plane either way,
so the guard stays.
- The reserved-upstream-header list is gone as a permission gate. Credential
and cookie names are forwardable when a pattern names them exactly, and
settable as `default_headers`; only protocol-breaking names and the
gateway's own namespace remain blocked outright.
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (13)
Comment |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes credential/header forwarding semantics across multiple proxy faces (including WebSocket handshakes), which is security-sensitive and merits final human review despite strong test coverage.
Pull request overview
This PR tightens and completes forward_client_headers behavior across proxy surfaces to prevent unintended credential/header leakage (especially on passthrough routes with operator-chosen auth slots) and to apply consistent forwarding to /v1/realtime’s upstream WebSocket handshake.
Changes:
- Extend the “exact-name only” rule to include per-route credential/identity slots on passthrough routes (
auth_header_name,identity_header) so globs don’t forward them. - Apply ProviderKey
request.forward_client_headersto/v1/realtimeupstream handshakes, while explicitly blocking WebSocket handshake-negotiation headers. - Add/expand regression coverage (unit + e2e), and pin Bedrock’s reserved header list to prevent doc/code drift.
File summaries
| File | Description |
|---|---|
| tests/e2e/src/cases/forward-client-headers-e2e.test.ts | Adds end-to-end coverage for passthrough per-route slots and ensemble sub-dispatch forwarding behavior. |
| schemas/resources/provider_key.schema.json | Updates forward_client_headers description (Bedrock reserved headers + compatibility note). |
| schemas/resources/passthrough_route.schema.json | Documents per-route slot behavior (auth_header_name/identity_header) under forward_client_headers. |
| crates/aisix-proxy/src/realtime.rs | Wires forward_client_headers into realtime WS handshake with explicit blocked handshake slots + adds tests. |
| crates/aisix-proxy/src/passthrough_route.rs | Implements per-route exact-name slots by routing patterns through forward_pattern_admits_with(...) + adds tests. |
| crates/aisix-provider-bedrock/src/wire.rs | Pins Bedrock reserved header list against the public description to prevent drift. |
| crates/aisix-gateway/src/upstream_headers.rs | Introduces per-surface blocked header names in UpstreamHeaderContext and threads it into forwarding resolution. |
| crates/aisix-gateway/src/bridge.rs | Sets surface_blocked: &[] for standard HTTP bridge contexts. |
| crates/aisix-core/src/models/provider_key.rs | Updates Rust doc comment to reflect Bedrock reserved header list and compatibility note. |
| crates/aisix-core/src/models/passthrough_route.rs | Updates Rust doc comment to describe per-route slot behavior for forwarding patterns. |
| crates/aisix-core/src/lib.rs | Re-exports new “*_with” forwarding helpers. |
| crates/aisix-core/src/forwarded_headers.rs | Adds surface-slot-aware exact-match/forwarding helpers and tests. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…he realtime block list really stops Review follow-ups, no behavior change. The passthrough route's slot list was a collected `Vec` built per request for at most two entries. It is a fixed array now, with `""` standing for an unset slot — a header name is never empty, on the wire or in the schema, so the sentinel matches nothing and needs no filtering out. Pinned by a test, since the code now depends on it. The `REALTIME_HANDSHAKE_SLOTS` comment claimed relaying `sec-websocket-key` or `-version` would break the upstream handshake. Delivery already declines a non-credential name the outbound request carries, and it carries both, so they are listed for completeness rather than because a pattern can reach them today. `sec-websocket-protocol` and `-extensions` are the two that genuinely are reachable — nothing else declines them — and the comment now says which is which.
…ake the controls real Review round. One behavior fix, the rest are tests and prose. `/v1/realtime` renders its upstream handshake as text and refuses a header value it cannot read as a string, failing the whole connection rather than that one header. Header values may legally carry 0x80-0xFF and hyper accepts them inbound, so once this face started forwarding client headers a caller sending `x-user-name: José` could no longer open a realtime session at all — while the same header is forwarded byte-for-byte on every other face. Such an entry is now dropped, and the session opens without it. Reproduced over a raw socket, because the WebSocket client used in tests renders the outbound handshake as text too and cannot send the header in the first place. The control assertions in the passthrough tests were checking a header the route never strips. A passthrough route relays whatever it does not strip, so those lines held whether or not a pattern matched — they could not show what their comments claimed they showed. Every control is now a header the ProviderKey strips and the glob recovers, which also makes the `gateway_key` case pin what it was written for: `x-gw-key` IS in that route's strip set, so the pattern is genuinely asked about it and answers yes because THIS route declared no slot. Widen the narrowing to a global list and that test fails. `/v1/realtime` also had no coverage at the real-binary-plus-etcd layer; the existing case now asserts a named header reaches the upstream handshake while the browser flow's `sec-websocket-protocol` does not, even though the ProviderKey names it exactly. Two prose corrections. `x-amz-target` is not derived by the SigV4 signer — it is the operation selector the signature covers — so the description now says the signer OWNS all six names and drops any supplied value, matching how the control plane already words it. And the claim that the route schema forbids every shared credential name for `auth_header_name` / `identity_header` was wrong: it forbids five of them, and the two it allows are on the shared list already, which is what makes the union complete.
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 free
to 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.
Four follow-ups to the one auth-independent client-header forward, batched into one PR.
The exact-name rule was incomplete on passthrough routes
forward_client_headersforwards a credential slot only when a pattern names it exactly — a"*"or"x-*"glob is a statement about the operator's own headers, not consent to hand a third party the caller's credential. That rule was implemented against a fixed list of names, which is complete on/v1/*and/mcp, where the caller's gateway credential always arrives inAuthorizationorx-api-key.It is not complete on
passthrough_route, where the operator names the slot:auth_mode: header_keythe gateway credential arrives in the route'sauth_header_name, and route validation guarantees that name is never one of the fixed ones. Soforward_client_headers: ["x-*"]besideauth_header_name: x-gw-keyrelayed every caller's AISIX gateway key to the upstream, where it can be replayed against this gateway.identity_headerhas the same shape, against a field whose own description promises it is "stripped before forwarding".A route's own
auth_header_nameandidentity_headernow join the exact-name set for that route. A glob no longer reaches either; naming one in full still forwards it, exactly as before. No existing configuration changes meaning except one that was relaying a credential it never named. The two other auth modes name no slot of their own, so nothing joins the set there and a glob keeps meaning what it did./v1/realtimenever carried the settingOnly the shared dispatch path applied
forward_client_headers;/v1/realtimebuilds its upstream WebSocket handshake by hand. An operator who declared that an upstream reads the caller's own credential got that on every other/v1/*endpoint and silently not there.It now applies the same forward with the same precedence — a named credential slot displaces the ProviderKey's own rather than joining it on the wire — and refuses the handshake slots this surface owns:
sec-websocket-key,sec-websocket-version,sec-websocket-extensionsdescribe the connection the caller opened, not the one the gateway opens. Relaying the caller's extension offer would make the upstream enable a compression the gateway's own codec never negotiated.sec-websocket-protocolis worse than broken: the documented browser flow puts the caller's own AISIX key in it (openai-insecure-api-key.<key>), so relaying it would hand the provider the credential this gateway authenticates with. It also selects the subprotocol the gateway echoes back to the client, which is the gateway's answer to make.A header value the handshake cannot render as text is dropped rather than
failing the session: this face writes the handshake as text and refuses a
value it cannot read as a string, and header values may legally carry
0x80-0xFF. Every other face hands the bytes straight to the HTTP client.
request.default_headersremains unapplied on this face, as it always hasbeen. That is a separate feature with its own behavior change and its own
hazards on a handshake, so it is deliberately not in this PR; the module
doc now says so at the site rather than leaving it to be rediscovered.
The
ensembleandsemanticsub-dispatch paths were audited, and are already correctBoth bypass the routing trunk, so they were checked against the model-kind lockstep rule rather than assumed:
No behavior change here; the e2e coverage is new, so a future sub-dispatch cannot drop it silently.
The Bedrock reserved-name list in the schema description was wrong
The
forward_client_headersdescription said an AWS Bedrock upstream refuses four names. It refuses six:x-amz-targetandx-amzn-bedrock-acceptwere missing from the prose, and the latter is not a SigV4 input at all — it selects the response wire shape the gateway then decodes. The description now says six and separates the two reasons, and the reserved list is pinned by a test so prose and code cannot drift again.Behavior changes since 0.13.0
Recorded here because release notes are built from the commits.
/v1/realtimenow honours an already-released setting it used to ignore.provider_keys.request.forward_client_headershas shipped since 0.10.0. A deployment that already carries one —["authorization"], or a glob — forwarded nothing into a realtime handshake before this change and forwards the matching headers after it. With["authorization"]that means the caller's own AISIX gateway key now reaches the realtime provider, which is the same thing that setting already does on every other endpoint. Nothing to migrate; operators who scoped a provider key to realtime on the assumption that the setting did not reach it need to know.The two below come from the preceding change rather than this one.
default_headersfiltering widened. 0.13.0 filtered the block only against credential names, so gateway-namespace headers passed through. The filter now also excludes thex-aisix-prefix. An existingdefault_headers: {"x-aisix-foo": "..."}silently stops being delivered once the data plane moves past that change, and because the control plane re-validates the merged block, that provider key is rejected on edit until the header is removed. The error is explicit and the configuration is dead on the new data plane either way, so the guard stays — but operators carrying one need to know.The reserved-upstream-header list is gone as a permission gate. Credential and cookie header names are now forwardable when a pattern names them exactly, and settable as
default_headers. Only protocol-breaking names and the gateway's own namespace remain blocked outright. The list survives under a narrower name, guarding one thing: a caller-supplied request id may still never be read out of a credential header.The
forward_client_headersdescription now also notes that naming a credential slot needs a data plane new enough to honor it — an older one refuses those names outright, so the pattern has no effect there rather than a different one.Tests
Every fix has a test that fails without it, verified by mutation:
crates/aisix-core/src/forwarded_headers.rs— the predicate: a surface slot needs its own name, an ordinary header on the same surface still answers to a glob, and a surface that names no slot behaves exactly as before.crates/aisix-proxy/src/passthrough_route.rs— the wiring, against a real upstream: glob vs exact name forauth_header_nameand foridentity_header, and agateway_keyroute left unchanged. Each asserts an ordinaryx-header still rides through, so a passing "not forwarded" assertion cannot be a pattern that never fired.crates/aisix-proxy/src/realtime.rs— a named header on the upstream handshake, a named credential slot displacing the ProviderKey's own and riding alone, and the browser flow's credential never leaving the gateway even against a"*"pattern.crates/aisix-provider-bedrock/src/wire.rs— the reserved list pinned against the names the public description spells out.tests/e2e/src/cases/forward-client-headers-e2e.test.ts— the route slots and the ensemble panel + judge (buffered and streamed) end to end, against a real binary.Review notes
ensemble/semanticaudit produced no code change, so the coverage added for it is new tests over unchanged behavior./v1/realtimedoes not carry this setting. Both are tracked outside this repository and land with this effort.