Repository navigation
Deliver server data that arrives with the proxy handshake reply - #280
Open
disarray09 wants to merge 1 commit into
Open
disarray09 wants to merge 1 commit into
disarray09 wants to merge 1 commit into
Conversation
A destination that speaks first (SSH, SMTP, FTP, IMAP, MySQL) can have its first bytes arrive in the same read as the SOCKS5 reply or the HTTP CONNECT response. handle_proxy_session stops as soon as the handler reports the connection established; at that point those bytes are still in the handler's buffers (SOCKS5 has not yet moved them from its server input to its client output, HTTP has moved them but nothing reads the client output), and they were never written to the client. The client saw a stream with its first bytes missing. After the handshake, run the handler's established step once with an empty server event and write whatever is bound for the client before the copy loops start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot stopped reviewing on behalf of
ssrlive due to an error
November 8, 2026 10:23
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
3 open issues
What changed in this PR
This PR ensures any “early” server bytes that arrive in the same read as the proxy handshake reply are forwarded to the client before the main relay loop begins.
Changes:
- Clone the proxy handler so it can be reused after
handle_proxy_session. - Add
take_early_server_datato drain buffered server→client bytes left behind after handshake establishment. - Add Tokio tests covering SOCKS5 and HTTP CONNECT scenarios where early server data piggybacks on the handshake response.
| File | Description |
|---|---|
| src/lib.rs | Flushes and forwards early server data post-handshake; adds targeted async tests validating the behavior. |
Review effort: Lite
Comment on lines
+1029
to
+1042
| /// `handle_proxy_session` stops as soon as the handler reports the connection | ||
| /// established. Any bytes that followed the reply in the same read are left in | ||
| /// the handler's server-side input buffer, because the handler only moves that | ||
| /// buffer to its client-bound output on its next `push_data`. Pushing an empty | ||
| /// server event runs that step once, and the client-bound output is drained. | ||
| async fn take_early_server_data(proxy_handler: &Arc<Mutex<dyn ProxyHandler>>) -> crate::Result<Vec<u8>> { | ||
| let mut handler = proxy_handler.lock().await; | ||
| let event = IncomingDataEvent { | ||
| direction: IncomingDirection::FromServer, | ||
| buffer: &[], | ||
| }; | ||
| handler.push_data(event).await?; | ||
| let data = handler.peek_data(OutgoingDirection::ToClient).buffer.to_vec(); | ||
| handler.consume_data(OutgoingDirection::ToClient, data.len()); |
Comment on lines
+1034
to
+1042
| async fn take_early_server_data(proxy_handler: &Arc<Mutex<dyn ProxyHandler>>) -> crate::Result<Vec<u8>> { | ||
| let mut handler = proxy_handler.lock().await; | ||
| let event = IncomingDataEvent { | ||
| direction: IncomingDirection::FromServer, | ||
| buffer: &[], | ||
| }; | ||
| handler.push_data(event).await?; | ||
| let data = handler.peek_data(OutgoingDirection::ToClient).buffer.to_vec(); | ||
| handler.consume_data(OutgoingDirection::ToClient, data.len()); |
Comment on lines
+1041
to
+1042
| let data = handler.peek_data(OutgoingDirection::ToClient).buffer.to_vec(); | ||
| handler.consume_data(OutgoingDirection::ToClient, data.len()); |
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.
Problem
A destination that speaks first — SSH, SMTP, FTP, POP3, IMAP, MySQL — can lose up to one read's worth of its first bytes.
handle_proxy_sessionreads the proxy's handshake reply into a 4096-byte buffer and stops as soon as the handler reports the connection established. If the destination's first bytes arrive in the same read as the SOCKS5 reply (or the HTTP200toCONNECT), they are still in the handler at that point:SocksProxyHandlerhas parsed the reply and switched toEstablished, but only movesserver_inbuftoclient_outbuf(relay_traffic) on its nextpush_data, which never comes.HttpConnectionhas moved them toclient_outbuf, but nothing reads the client-bound buffer after the handshake.handle_tcp_sessionthen starts the copy loops, and those bytes are never written to the client. The connection stays open, so the client sees a stream with its first bytes missing — e.g. an SSH client that never receives the server's version banner.Whether it happens depends on how the reply and the first data coalesce, so it is intermittent. Behind a local SOCKS5 server on loopback between 0 and 3 of every 10 connections to a server-first target lost 1450 or 4086 bytes. Client-first protocols (HTTP, TLS) are not affected, because the server has nothing to send before the client speaks.
Fix
After
handle_proxy_sessionsucceeds,take_early_server_datapushes one emptyFromServerevent so the handler runs its established step, then drainsToClient.handle_tcp_sessionwrites those bytes to the client before the copy loops start. It is a no-op when nothing followed the reply.Tests
New
early_server_data_testsinsrc/lib.rs:socks5_data_read_with_the_reply_is_kept— SOCKS5 reply and an SSH banner in one read; the banner is returned exactly once. Fails without the fix.http_connect_data_read_with_the_response_is_kept—200 Connection establishedfollowed by an SMTP greeting in one read.a_bare_reply_leaves_nothing_behind— no extra data, nothing returned.cargo test --lib(18 passed),cargo clippy --lib --tests -- -D warningsandrustfmt --checkare clean. We also run this change in a downstream project's end-to-end tests (fake TUN with a real TCP stack, real SOCKS5 server): 0 bytes lost in 30 server-first connections, against 10–30% before.🤖 Generated with Claude Code