[esnible]

What

Teach the H1 server session that a 2xx response to a CONNECT request establishes a tunnel (RFC 9110 section 9.3.6), by mirroring the existing 101 upgrade path in prepare_response_header:

• New is_connect_tunnel_resp(&self, header) helper: request method is CONNECT and response status is 2xx.
• In prepare_response_header, a connect-tunnel response:
  • disables keepalive before the Connection header value is chosen, so the response is emitted with Connection: close;
  • sets upgraded = true and converts the body reader to close-delimited — via init_body_reader() (seeding from preread_body) when not yet initialized, or convert_to_close_delimited() (adopting the overread buffer) when it was, so bytes the client pipelined behind the CONNECT head are served as the first tunnel data;
  • the existing is_upgrade branch becomes else if, leaving the 101 path unchanged.
• Defense-in-depth: init_body_writer uses a close-delimited writer for connect-tunnel responses.

Non-2xx responses to CONNECT (e.g. a proxy policy denial like 403) keep ordinary request/response framing.

Why

This is the session primitive needed for forward-proxying (praxis-proxy/praxis#781). The allow_connect_method_proxying gate already lets CONNECT reach the ProxyHttp app, but a CONNECT 200 left the session in ordinary framing: the request was framed Content-Length: 0 (RFC 9112 section 6.3), so client-to-origin tunnel bytes were never read, and the 200 risked chunked framing being injected downstream. Without a raw-stream escape hatch on HttpSession, the only workable mechanism is to make the 2xx itself engage the same tunnel machinery the 101 path already provides.

Downstream (the praxis proxy app) can then pump tunnel bytes through stable public APIs: read_request_body() returns raw tunnel bytes including pipelined preread, write_response_body(bytes, end) writes raw bytes.

Testing

New unit tests in tests_stream (modeled on the 101/upgrade tests):

• connect_tunnel_resp_converts_session_to_tunnel — 2xx sets was_upgraded(), body reader is UntilClose, pipelined bytes are served first, Connection: close is emitted.
• connect_tunnel_resp_after_body_reader_initialized — the framed-CL(0)-then-tunnel path; overread bytes still become tunnel data.
• connect_resp_non_success_keeps_framed_session — 403 does not convert the session.

Results on macOS (aarch64):

• cargo test -p quixotic-plecostomus-core --lib — 563 passed, 0 failed (includes the existing 101/upgrade regression tests, unchanged).
• cargo test -p quixotic-plecostomus-proxy --lib — 58 passed.
• cargo clippy -p quixotic-plecostomus-core — clean; cargo fmt --check — clean.
• Pre-existing, unrelated: pingora-proxy test_basic integration tests require openresty (not installed locally); cargo clippy --all-features fails inside the third-party dial9-tokio-telemetry dependency.

[coderabbitai]

Important

• Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Central YAML; Path: .coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: cd680a37-3fdd-424b-a02d-b50ce494f4d8

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[araujof]

Thanks for this PR! I found a few issues that need fixing before merging.

It would help to create tests for these issues alongside the passing tests for refusals, response and tunnel bytes in one read, and disabled connection reuse after CONNECT 200/204.

[araujof]

H2 DATA comes through as Body, so setting seen_upgraded here causes the next task to be rejected. This fails on the PR and passes on main. The subrequest case also panics on UpgradedBody. Can we check the protocols on both sides and return a normal error for combinations we don't support before sending the 2xx? H1->H2 needs handling too.

[esnible]

Fixed in cd4b2cd. A CONNECT tunnel is now only allowed h1→h1 (both halves switch to raw bytes) or h2→h2 (both stay framed). reject_unsupported_connect_tunnel refuses any other pair, including subrequest and custom downstreams and both h1↔h2 directions, with a 502. It runs after the request filters that can change the method and before anything is sent upstream or downstream, so the subrequest UpgradedBody panic is unreachable. The h2-downstream seen_upgraded case was fixed in 36a3946 (ServerSession::is_connect_tunnel_resp is h1-only). Tests: session-type pairs, the 502 (also when a filter turns a GET into CONNECT), and a subrequest CONNECT refused before the upstream.

[araujof]

response_filter can turn the upstream 200 into a 403 after we've switched to tunnel mode here. We then send the 403 and reject the next UpgradedBody. I also tried 403->200: we send the 200, then reject Body. I'd keep the original upstream method and status and check the downstream decision after filtering, before writing the header.

[esnible]

Fixed in cd4b2cd, as you suggested. The session records the upstream's own tunnel decision from its original response, before any filter runs (ConnectTunnelStatus). downstream_response_task_filter compares it with the final status before writing the header, and returns InvalidHTTPHeader on a mismatch, so 200→403 and 403→200 both fail with nothing written. Changes within the same decision (200→204, 403→407) are still allowed. Tests cover both directions with the header alone, and end to end through h1_response_filter with a real response_filter.

[araujof]

Sending the CONNECT head and tunnel bytes in one read works. Sending the same bytes in two reads fails with Sent data after end of body while we're waiting for the upstream response (read_body_or_idle(true), default settings). Could we keep those bytes in overread until CONNECT succeeds? The result shouldn't depend on how the bytes are split across reads.

[esnible]

Fixed in 24653ae. While a CONNECT isn't established yet, the idle branch of read_body_or_idle keeps the bytes in overread, where one-read bytes already are, and stays pending. A 2xx then serves them first through convert_to_close_delimited. It initializes the body reader first, otherwise a later init would replace the overread and drop the bytes. Keepalive is turned off, so a refused CONNECT closes the connection instead of parsing them as a request, and it runs before the pipelining branch. Unit tests in test_connect_early_data; test_connect_tunnel_bytes_in_a_later_read in test_basic reproduces your Sent data after end of body without the fix.

[araujof]

I enabled gzip and sent CONNECT with Accept-Encoding: gzip. With a 200 carrying Content-Type: text/plain, the tunnel data came out gzip encoded. UpgradedBody still goes through the downstream response_body_filter. Should compression and decompression stop when CONNECT succeeds, as they do for 101? Refused CONNECT requests would still get normal response compression.

[esnible]

Fixed in 9494678. ResponseCompressionCtx now records whether the request is CONNECT. It does this even while disabled, so a ctx enabled later still knows. A 2xx to CONNECT is then treated like 101: no compression or decompression for the rest of the connection, and will_decompress agrees. Since the ctx is shared, this covers the downstream module and the proxy's upstream compression. Refused CONNECT responses are still compressed. Tests at the ctx, module, and integration level (test_connect_tunnel_not_compressed, your gzip + text/plain case).

[araujof]

h1_response_filter adds Transfer-Encoding: chunked to a CONNECT 200 with no Content-Length. That header survives even though the writer sends raw tunnel bytes. RFC 9110 §9.3.6 says successful CONNECT responses must omit both fields. Removing them here would also cover headers added by the proxy filters. Those filters should skip adding chunked encoding for CONNECT success too.

This could replace the existing if block:

Suggested change

	if self.is_connect_tunnel_resp(header) {
	self.set_keepalive(None);
	}
	if self.is_connect_tunnel_resp(header) {
	header.remove_header(&header::CONTENT_LENGTH);
	header.remove_header(&header::TRANSFER_ENCODING);
	self.set_keepalive(None);
	}

[esnible]

The prepare_response_header strip landed in 36a3946, matching your suggestion: it removes CL and TE and calls set_keepalive(None). In 48f9b37 the h1, h2 and custom response filters also stop adding chunked to a CONNECT 2xx, through one shared response_has_no_body helper. Test: h1_response_filter_chunks_only_body_bearing_responses (CONNECT 200/204 get no TE; CONNECT 403 and GET 200 still do).

[araujof]

CONNECT 200 with Content-Length: ignored is rejected in validate_response() before we get here. RFC 9110 §9.3.6 requires us to ignore Content-Length and Transfer-Encoding on these responses. An early return there would let this through while keeping length validation for refusals and other responses.

In validate_response(), just after getting resp_header:

        if self.is_connect_tunnel_resp(resp_header) {
            return Ok(());
        }

[esnible]

Fixed in 48f9b37 with your suggested early return in validate_response. That function only does the CL check, and header bytes are validated earlier, so nothing else is skipped. Tests: invalid, conflicting and TE framing on a CONNECT 200 are accepted and the tunnel stays close-delimited; a CONNECT 403 with an invalid CL is still rejected.

[araujof]

The docs above still say Upgrade+101 is required for was_upgraded() to return true. CONNECT+2xx now qualifies too. Could we update the H1 client/server docs and the wrapper wording? For the H1 methods:

    /// Whether this request switched protocols through an Upgrade handshake
    /// or established a tunnel through a successful CONNECT response.

The wrapper should mention that H2 still returns false.

[esnible]

Updated in 48f9b37: the H1 client and server was_upgraded() docs use your wording. The ServerSession wrapper doc now says it is always false for HTTP/2, whose CONNECT tunnels stay framed. I also updated the task-filter doc and its error messages to mention the CONNECT tunnel.

[esnible]

Thanks for the review. I pushed six commits on top of 2412a6e; each inline thread names the commit that addresses it.

Tests you asked for:

• Refusals: a CONNECT 403 keeps ordinary framing on both halves and is still compressed. A 403 with an invalid Content-Length is still rejected, and a refusal after early tunnel bytes closes the connection.
• Response and tunnel bytes in one read: test_connect_response_and_tunnel_bytes_in_one_read. The client half also has connect_2xx_resp_switches_to_tunnel.
• Request head and tunnel bytes in one read: test_connect_tunnel_bytes_in_the_request_head_read. The two-read case is test_connect_tunnel_bytes_in_a_later_read.
• Connection reuse disabled after CONNECT 200/204: connect_2xx_resp_disables_connection_reuse (200, 204, and 200 with Connection: keep-alive).

Each fix was mutation-checked: with the fix reverted, its tests fail.

Locally on macOS, test_basic upstream tests need a 127.0.0.2 loopback alias (client_bind_to_ipv4 in pingora_conf.yaml). openresty isn't installed here, so I ran the CONNECT integration tests that don't need the mock origin; CI runs the rest.

🤖 Generated with Claude Code

[shaneutt]

One comment to resolve from me, then once all other comments are resolved 👍

[esnible]

Post-merge review findings

These are from a review of the head (f32800e), which is identical to the merge commit 1fac8f2. Line links point to 1fac8f2.

The must-fix is addressed in #8. The suggestions and nits are not fixed yet.

Must-fix

1. server.rs:1333

Fixed in #8. This comment says early tunnel bytes "must not depend on whether they shared a read with the head" and that "a refused CONNECT closes the connection". That only holds for bytes from a later read. When the early bytes arrive in the same read as the CONNECT head, init_content_length_owned(0, preread_body) (line 1206) puts them in overread. This branch never runs for them, so connect_early_bytes_stashed stays false and keepalive stays on.

Then on a 403/407 with pipelining enabled, reuse() (1541-1545) passes the overread to the next session as pipelined_prefix. Probe: CONNECT ... \r\n\r\nGET /x HTTP/1.1\r\nHost: a\r\n\r\n in one read, then a 407 with CL 0, and reuse() returns Some(prefix="GET /x ..."). A front end that treats post-CONNECT bytes as opaque tunnel data would see them run here as a request, which is the CONNECT early-data smuggling pattern. Without pipelining, reuse() does close (1537), but the 407 head still says Connection: keep-alive.

Suggested fix: in prepare_response_header, call set_keepalive(None) for any final non-2xx response to CONNECT before the Connection value is chosen (line 889). The narrower option is to do it only when preread or overread is non-empty. Test: same read + set_pipelining_enabled(true) + 407, asserting reuse() is None and the head says Connection: close.

Suggestions

2. proxy_h1.rs:340

A CONNECT that carries a request body is still forwarded with its framing. validate_request (server.rs:492) accepts Content-Length/Transfer-Encoding on CONNECT, and finalize_h1_upstream_request_framing has no CONNECT case. RFC 9110 §9.3.6 says CONNECT request content has no defined semantics. Two results:

• New in this PR: with Transfer-Encoding: chunked and a fast 2xx, this branch now deliberately lets the late Body end marker reach send_body_to1 -> finish_body, and that writes 0\r\n\r\n into the raw tunnel.
• Already on main: with Content-Length: N, the upstream receives CONNECT plus N body bytes. If it refuses with a keep-alive 403, the connection is pooled, and the upstream may read those client-controlled bytes as the start of the next request.

The cheapest fix is to reject CONNECT with CL>0 or any TE (400) in validate_request. That closes both cases.

3. server.rs:1300

No test covers || self.connect_early_bytes_stashed. I removed it and all 16 CONNECT/early-data tests still pass. poll_idle_read_pending re-polls the same future, which is already parked in pending(), so the flag is never checked again. In the proxy, each select! iteration creates a new future. Without this short-circuit, each iteration reads more bytes and pushes them to overread. That is unbounded buffering before the upstream answers, and the client's TCP backpressure is lost. Byte order is unchanged, so the order assertions can't detect it. Suggested test: drop the future, call read_body_or_idle(true) again, and assert that the mock's next read was not consumed.

4. mod.rs:256

No test covers "Recorded even while disabled". I moved the connect_req recording below the is_enabled() early return and all three CONNECT compression tests still pass, because each one builds a ctx that is already enabled. Suggested test: a fully disabled ctx (level 0, no decompression) sees the CONNECT, then adjust_decompression(true), then a 200 with Content-Encoding: gzip, asserting that nothing is decompressed.

5. lib.rs:1144

"Either is refused" is only true for HTTP/1. proxy_h2.rs:235 passes upstream_can_tunnel=false, so upstream_tunnel is always Some(false) on h2. The downstream h2 session never reports a tunnel either, so this function can't fire for h2->h2. A response_filter that rewrites an origin's 403 to 200 tells the h2 client a tunnel is open and delivers the 403 body as tunnel bytes. A 200 rewritten to 403 sends tunnel bytes as the 403 body. Framing stays safe because both sides are framed, but the guarantee is weaker than documented. Either record CONNECT && 2xx from the h2 upstream and compare it with the final downstream status, or narrow this doc to HTTP/1.

6. test_basic.rs:1245

This test doesn't reach the proxy-task write path. Uncached upstream tasks always go through write_response_tasks (proxy_h1.rs:455), which doesn't use the proxy-task queue. send_downstream_proxy_task is only called in the serve-from-cache branch (proxy_h1.rs:716-717), which a CONNECT never reaches. So this repeats the default test, and the 36a3946 message ("default and proxy-task write paths") claims more than it covers. Either drop it, or add a send_downstream_proxy_task unit test for CONNECT next to the 101 ones at lib.rs:2979.

7. test_basic.rs:980

The round-trip tests check exact tunnel bytes, but nothing checks teardown. A regression that leaves a tunnel half-open, or keeps the downstream connection alive after a tunnel, would pass. Suggested additions: after the echo, close the upstream socket and assert read() == 0 on the client within the deadline, check the reverse direction, and assert connection: close in the 2xx head.

Nits

8. lib.rs:1153

This early return also skips the check when upstream_tunnel is already Some(true). A real 1xx can't arrive after the upstream's final status is recorded, so that state only happens when a filter rewrote the final 2xx to 1xx (for example 103). The 1xx head is written, and the next UpgradedBody then fails with "upgraded body before upgrade". No bytes leak, but it breaks "refused before the header is written". Fix: skip informational statuses only while upstream_tunnel is None.

9. server.rs:912

The tests never run the need_init() branch. With the default close_on_response_before_downstream_finish = true, the is_body_done() call at line 860 always sets up the reader first. I put a panic! here and every test still passed. So connect_tunnel_resp_converts_session_to_tunnel and connect_tunnel_resp_after_body_reader_initialized both go through convert_to_close_delimited. A variant with set_close_on_response_before_downstream_finish(false) would cover this branch.

10. mod.rs:710

This comment belongs to decide_action. Adding is_connect_tunnel left it above the wrong function.

11. test_basic.rs:1058

This assert runs in a spawned task, so a failure doesn't fail the test. It shows up as a 5s "CONNECT response head" timeout instead. The assert also only sees bytes that share a read with the head. Returning the JoinHandle from connect_upstream and awaiting it would surface the real failure. Related: the sleep-ordered tests (line 1144, test_connect_tunnel_survives_request_end_after_fast_2xx) won't flake toward failure, but on a slow CI host they can pass without hitting the ordering they target.

12. lib.rs:2936

body.data().await here and client.await at line 2973 have no timeout, so a regression would hang CI instead of failing. Wrapping them in tokio::time::timeout would fix this.

13. test_basic.rs:905

The comment at line 887 ("CONNECT 2xx responses are not allowed to have response bodies, so this is non-standard behavior") is now out of date. This PR strips the Content-Length, so "ok" is tunnel data rather than a body.

14. proxy_h1.rs:1342

"the same steps as process_upstream_tasks" is too strong. The helper skips the compression and upstream-modules steps.

Assisted-By: Claude Code