-
Notifications
You must be signed in to change notification settings - Fork 321
fix(proxy): strip unsafe HTTP headers before owner-bridge forward #1036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
|
|
||
| import aiohttp | ||
|
|
||
| from app.core.clients.proxy import ProxyResponseError | ||
| from app.core.clients.proxy import ProxyResponseError, filter_inbound_headers | ||
| from app.core.config.settings import get_settings | ||
| from app.core.crypto import get_or_create_key | ||
| from app.core.errors import OpenAIErrorEnvelope, openai_error, response_failed_event | ||
|
|
@@ -22,6 +22,26 @@ | |
| from app.modules.api_keys.service import ApiKeyUsageReservationData | ||
| from app.modules.proxy._service.http_bridge.helpers import _http_bridge_request_budget_seconds | ||
|
|
||
| # HTTP-only and hop-by-hop headers that must not be forwarded through the | ||
| # internal bridge. These headers are either illegal in WebSocket handshakes or | ||
| # carry HTTP framing semantics that the aiohttp upstream session manages itself. | ||
| # Applies on top of filter_inbound_headers (which already strips authorization, | ||
| # host, content-length, and x-forwarded-* / cf-* headers). | ||
| _BRIDGE_UNSAFE_HEADER_NAMES = frozenset( | ||
| { | ||
| "accept", | ||
| "accept-encoding", | ||
| "connection", | ||
| "content-type", | ||
| "cookie", | ||
| "keep-alive", | ||
| "te", | ||
| "trailer", | ||
| "transfer-encoding", | ||
| "upgrade", | ||
| } | ||
| ) | ||
|
|
||
| HTTP_BRIDGE_INTERNAL_FORWARD_PATH = "/internal/bridge/responses" | ||
| HTTP_BRIDGE_FORWARDED_HEADER = "x-codex-bridge-forwarded" | ||
| HTTP_BRIDGE_ORIGIN_INSTANCE_HEADER = "x-codex-bridge-origin-instance" | ||
|
|
@@ -136,9 +156,8 @@ def build_owner_forward_headers( | |
| payload: ResponsesRequest, | ||
| context: HTTPBridgeForwardContext, | ||
| ) -> dict[str, str]: | ||
| forwarded = dict(headers) | ||
| forwarded.pop("host", None) | ||
| forwarded.pop("content-length", None) | ||
| filtered = filter_inbound_headers(headers) | ||
| forwarded = {key: value for key, value in filtered.items() if key.lower() not in _BRIDGE_UNSAFE_HEADER_NAMES} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a downstream request uses Useful? React with 👍 / 👎. |
||
| forwarded[HTTP_BRIDGE_FORWARDED_HEADER] = "1" | ||
| forwarded[HTTP_BRIDGE_ORIGIN_INSTANCE_HEADER] = context.origin_instance | ||
| forwarded[HTTP_BRIDGE_TARGET_INSTANCE_HEADER] = context.target_instance | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In deployments with
api_key_auth_enabled=True, forwarded owner requests need the clientAuthorizationheader on the internal POST becauseinternal_bridge_responses()calls_validate_internal_bridge_api_key(request)before stripping headers for the upstream call._forward_http_bridge_request_to_owner()deliberately re-adds that header via_headers_with_authorization(...), but this newfilter_inbound_headers()call removes it again, so remote owner forwarding now reaches/internal/bridge/responseswithout auth and is rejected before the signed request can be streamed. Preserve the auth header for the internal bridge hop and strip it only before upstream forwarding.Useful? React with 👍 / 👎.