fix(proxy): strip unsafe HTTP headers before owner-bridge forward#1036
fix(proxy): strip unsafe HTTP headers before owner-bridge forward#1036ozpool wants to merge 2 commits into
Conversation
build_owner_forward_headers forwarded the full downstream header map, which could carry hop-by-hop and HTTP-only headers (accept, accept-encoding, connection, content-type, cookie) into the internal bridge POST and from there into the upstream Responses WebSocket handshake on the owner node. Apply filter_inbound_headers first (removes authorization, host, content-length, x-forwarded-*, cf-* headers) then additionally drop _BRIDGE_UNSAFE_HEADER_NAMES (hop-by-hop and HTTP framing headers that must not flow to a WebSocket connection). Regression tests added to test_http_bridge_forwarding.py. Fixes Soju06#1016
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ac797d9f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| forwarded = dict(headers) | ||
| forwarded.pop("host", None) | ||
| forwarded.pop("content-length", None) | ||
| filtered = filter_inbound_headers(headers) |
There was a problem hiding this comment.
In deployments with api_key_auth_enabled=True, forwarded owner requests need the client Authorization header on the internal POST because internal_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 new filter_inbound_headers() call removes it again, so remote owner forwarding now reaches /internal/bridge/responses without 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 👍 / 👎.
| 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.
When a downstream request uses Connection: X-Foo with an accompanying X-Foo header, this code removes only Connection and the fixed unsafe names, leaving X-Foo in the internal bridge request. Before this change the owner-side websocket header builder could see Connection and drop token-nominated hop-by-hop headers; after this point it no longer can, so those per-connection headers are forwarded upstream despite being marked hop-by-hop. Capture the Connection tokens before deleting the header and exclude those names as well.
Useful? React with 👍 / 👎.
The owner instance re-validates the client API key from the Authorization header (_validate_internal_bridge_api_key) before swapping in its own upstream access token, so routing build_owner_forward_headers through filter_inbound_headers broke forwarded bridge requests whenever api_key_auth_enabled is set: the header was stripped before the owner could validate it. Re-add Authorization after filtering, and also drop any header named by the inbound Connection header to honor the hop-by-hop contract.
|
Thanks for the catch on both, addressed in 8ffb546:
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Friendly ping on this one — it's been sitting green since the fix went in. Codex's last review ran against |
|
To use Codex here, create a Codex account and connect to github. |
2 participants
Summary
build_owner_forward_headerscopied the full downstream header map and only manually removedhostandcontent-length. Hop-by-hop and HTTP-only headers —accept,accept-encoding,connection,content-type,cookie— were forwarded into the internal bridge POST and from there flowed through to the upstream Responses WebSocket handshake on the owner node.Type of change
fix:— bug fix (no behavior change beyond the bug)Linked issue: Closes #1016
OpenSpec
Changes
filter_inbound_headersintohttp_bridge_forwarding.py_BRIDGE_UNSAFE_HEADER_NAMESconstant listing hop-by-hop and HTTP framing headers (accept,accept-encoding,connection,content-type,cookie,keep-alive,te,trailer,transfer-encoding,upgrade) that must not be forwarded to an upstream WebSocket pathdict(headers)+ manual pops inbuild_owner_forward_headerswithfilter_inbound_headers(stripsauthorization,host,content-length,x-forwarded-*,cf-*) followed by a second pass over_BRIDGE_UNSAFE_HEADER_NAMESauthorization/host/content-lengthare dropped while safe headers pass throughTest plan
Checklist
<type>(<scope>)?: <subject>).uv run pytest tests/unit/locally — all pass.