feat: forward client request headers to upstream providers in bridge routes#214
feat: forward client request headers to upstream providers in bridge routes#214ssncferreira wants to merge 1 commit intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a2da2f0 to
252f03a
Compare
252f03a to
2d43f46
Compare
| // TODO(ssncferreira): inject actor headers directly in the client-header | ||
| // middleware instead of using SDK options. |
There was a problem hiding this comment.
Will handle this in a follow-up in order to not increase the scope of this PR.
|
|
||
| // nonForwardedHeaders are transport-level headers managed by aibridge or | ||
| // Go's HTTP transport that must not be forwarded to the upstream provider. | ||
| var nonForwardedHeaders = []string{ |
There was a problem hiding this comment.
One thing I noticed is that Copilot sends a Cookie header (e.g. from API dump: Cookie: csrf...N0E=). I assume this is used for session purposes between Copilot and Github, but do you see any issue in forwarding as is upstream?
There was a problem hiding this comment.
I don't think we should make many judgment calls on these sorts of things. Whatever the client sends is what we should deliver.
There was a problem hiding this comment.
I'm not an expert on Cookies but from quick research I don't see why we shouldn't forward them as received.
|
|
||
| // nonForwardedHeaders are transport-level headers managed by aibridge or | ||
| // Go's HTTP transport that must not be forwarded to the upstream provider. | ||
| var nonForwardedHeaders = []string{ |
There was a problem hiding this comment.
I don't think we should make many judgment calls on these sorts of things. Whatever the client sends is what we should deliver.
| } | ||
|
|
||
| // authHeaders are headers that carry authentication credentials from the | ||
| // client. These are stripped because the SDK re-injects the correct |
There was a problem hiding this comment.
Nit: "the SDK re-injects" can be reworded to be more explicit.
Which SDK? Where does it do that?
(i know the answers but future readers may not)
|
|
||
| // SanitizeClientHeaders returns a copy of the client headers with hop-by-hop, | ||
| // transport, and auth headers removed. | ||
| func SanitizeClientHeaders(clientHeaders http.Header) http.Header { |
There was a problem hiding this comment.
I think Sanitize is perhaps the wrong term here. Maybe Prepare?
Since we're acting (almost) like a reverse-proxy, we should be adding in the X-Forwarded-For, X-Forwarded-Host, X-Forwarded-Proto headers here, no?
| mockUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| receivedHeaders = r.Header.Clone() | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
Nit: can we use a fixture instead? It'll make the tests a bit more concise.
pawbana
left a comment
There was a problem hiding this comment.
Overall LGTM although it is a bit hard to review new and changes to the tests 😅
|
|
||
| // nonForwardedHeaders are transport-level headers managed by aibridge or | ||
| // Go's HTTP transport that must not be forwarded to the upstream provider. | ||
| var nonForwardedHeaders = []string{ |
There was a problem hiding this comment.
I'm not an expert on Cookies but from quick research I don't see why we shouldn't forward them as received.
|
|
||
| var receivedHeaders http.Header | ||
|
|
||
| mockUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
Would internal/integrationtest/mockupstream.go be useful here. Probably new method that creates upstreamResponse from string/bytes would be need.
I see now it may not have been a good idea to make it private 😞
| func TestOpenAI_CreateInterceptor(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("ChatCompletions_ClientHeaders", func(t *testing.T) { |
There was a problem hiding this comment.
Sorry for begin pedantic about this but would it be possible to reformat this into table tests? 😅
IIUC both tests are doing basically the same but with different request path?
Maybe I'm just used to it but I feel table test format is much easier to understand and keep track of test cases. It usually also shortens the code.
From my experience AIs for some reason don't do write tests in table format first but when you ask for it directly in follow up prompt AI realizes it is possible or sometimes provides a reason why it is not worth it.

Description
AI Bridge acts as a reverse proxy between clients and upstream AI providers. However, bridge routes were dropping client request headers as the interceptors construct new HTTP requests via provider SDKs, which set their own headers and discard the originals.
This PR changes bridge routes to forward client request headers to upstream providers, so the upstream sees the same headers the client sent, except headers related to auth, transport, or managed by aibridge.
Changes
intercept/client_headers.gowithSanitizeClientHeadersandBuildUpstreamHeaders. Client headers are sanitized by stripping hop-by-hop (RFC 2616), transport (Host,Accept-Encoding,Content-Length), and auth (Authorization,X-Api-Key) headers.X-AI-Bridge-Actor-*): injected by aibridge as per-request SDK options to identify the requesting user to the upstream.This is an intermediate step toward removing the SDK from the HTTP transport path. By forwarding client headers directly, aibridge is no longer dependent on SDK-managed headers, making future SDK removal simpler.
Follow-up
X-Forwarded-For,X-Forwarded-Host,X-Forwarded-Proto), while bridge routes do not. Since AI Bridge behaves as a reverse proxy, this handling should be consistent across all requests.ExtraHeadersmechanism for Anthropic and Copilot: now redundant since all client headers are forwarded.Tests
Tested manually with:
Closes: #192