Skip to content

fix(proxy): strip hop-by-hop headers from upstream response#1720

Open
xyz3282836 wants to merge 1 commit intofarion1231:mainfrom
xyz3282836:fix/strip-hop-by-hop-response-headers
Open

fix(proxy): strip hop-by-hop headers from upstream response#1720
xyz3282836 wants to merge 1 commit intofarion1231:mainfrom
xyz3282836:fix/strip-hop-by-hop-response-headers

Conversation

@xyz3282836
Copy link
Copy Markdown

When forwarding upstream responses to downstream clients, hop-by-hop headers (e.g. connection, transfer-encoding, keep-alive) were passed through as-is. This caused axum/hyper to reject the response with "user sent unexpected header" and drop the connection, resulting in "stream disconnected" / "Empty reply from server" on the client side.

Fix: strip all RFC 2616 §13.5.1 hop-by-hop headers (including any extra headers listed in the Connection header value) before building the downstream response, in both streaming and non-streaming paths.

When forwarding upstream responses to downstream clients, hop-by-hop
headers (e.g. connection, transfer-encoding, keep-alive) were passed
through as-is. This caused axum/hyper to reject the response with
"user sent unexpected header" and drop the connection, resulting in
"stream disconnected" / "Empty reply from server" on the client side.

Fix: strip all RFC 2616 §13.5.1 hop-by-hop headers (including any
extra headers listed in the Connection header value) before building
the downstream response, in both streaming and non-streaming paths.
Copy link
Copy Markdown
Owner

@farion1231 farion1231 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. I think the direction of this PR is good, and the underlying issue is worth fixing, because forwarding hop-by-hop response headers downstream can indeed trigger axum/hyper errors like user sent unexpected header, which may surface as stream disconnected or Empty reply from server.

That said, could you please take a look at the following issues before merging?

  1. This PR currently fixes the generic passthrough path in process_response, but it does not cover the Claude/OpenRouter transform path in handle_claude_transform. In that path, the non-streaming branch still forwards most upstream response headers, and the streaming branch still explicitly sets Connection: keep-alive, so the same class of issue may still happen there.

  2. The new stripped header set still seems incomplete. In particular, proxy-connection is not removed, and hyper also treats it as an illegal connection-specific header in HTTP/2. That means responses coming through certain proxies or gateways could still reproduce the same downstream failure.

I think this PR is a good start, but at the moment it looks more like a partial fix than a complete one. It would be great to cover the transform path as well and add some tests for streaming, non-streaming, and transformed responses so the behavior is fully locked down. I'd like to hear your opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants