Repository navigation
Fix proxy passthrough on Windows - #779
Conversation
[Medium impact] The change reaches the HTTPS proxy workflow and the regular Rust test pipeline. The PR appears safe to merge, with a non-blocking timing issue in the new test. Findings
SummaryThe proxy now waits for client request bytes before clearing TLS failures. Two abrupt closes after a completed handshake can therefore enable passthrough on Windows.
|
| for _ in 0..2 { | ||
| let tls = hangs_up.connect(name.clone(), h.connect_tunnel(port).await).await.unwrap(); | ||
| drop(tls); | ||
| tokio::time::sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
The new test waits only 100 ms for the proxy to count each hang-up. If the proxy has not counted both before the final connection starts, it still presents its own certificate and the test fails despite correct behavior. Wait for an explicit signal or use a bounded retry, as pinned_client_falls_back_to_passthrough already does.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
hang_up_after_the_handshake_passes_through slept a fixed 100 ms after each dropped connection and assumed the proxy had counted it. Poll a hidden ProxyHandle::handshake_state hook with a bounded timeout instead, so the final upstream-only connection is made once, after both hang-ups are recorded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The proxy now handles Windows clients that reject its certificate, and regular CI has a Windows job that runs the Rust tests the same way the release build does.
Root cause: The test isn't flaky, and it isn't the WSAECONNRESET theory. The reqwest client in the test uses native-tls (the workspace
reqwestenablesnative-tls, andTlsBackend::default()in reqwest 0.13.5 picks NativeTls whenever__native-tlsis on). On Windows, native-tls means SChannel.schannel-0.1.29turns off SChannel's own certificate check (ISC_REQ_MANUAL_CRED_VALIDATIONinINIT_REQUESTS,src/lib.rs:79). It runs its own check later, ininitialize()(src/tls_stream.rs:580):write_out()sends the client's last handshake flight, Finished included, and only then isvalidate()called. When the check fails, it returns an error and drops the socket without sending an alert. From the proxy's side,start.into_stream(config)ininterceptreturnsOk, sorecord_handshake_successcleared the failure count, and the hang-up that followed went toserve_http1as an ordinary I/O error. The host never reached two failures in a row, so it never switched to passthrough. On macOS, Secure Transport checks the certificate mid-handshake and hangs up before finishing, which the existing two-in-a-row rule already covered. The rustls test passed on Windows because rustls checks the certificate before its Finished and sends an alert.The fix: In
crates/yaak-proxy/src/request.rs, a connection only counts as a success once the client sends its first request bytes. These are read up front and replayed intoserve_http1through the existingRewindwrapper. If the client's first read after the handshake fails withUnexpectedEof(closed withoutclose_notify),ConnectionResetorConnectionAborted, that is recorded as a failure that needs two in a row (certificate_rejected: false), the same as a bare hang-up mid-handshake. A cleanclose_notifywith no request counts as neither, and other errors are returned as before. The logging moved into a smallrecord_handshake_failurehelper so both paths share it, and the doc comment onEngine::record_handshake_failurenow mentions Windows.Regression test:
hang_up_after_the_handshake_passes_throughincrates/yaak-proxy/tests/proxy.rscopies SChannel's behaviour on any OS: a rustls client completes the handshake with the proxy and drops the connection twice, then a client that trusts only the upstream CA must get through. It failed onmainbefore the fix (InvalidCertificate(UnknownIssuer), because the proxy was still intercepting) and passes after it. The existing tests and their assertions are unchanged.Windows CI job:
.github/workflows/ci.ymlhas a newtest-windowsjob onwindows-latest. Its steps matchrelease-app.yml's Windows path up to "Run Rust Tests": checkout,setup-vpwith Node 24, Rust stable,rust-cachewithshared-key: ci,arduino/setup-protoc@v3,vp install, andnpm run bootstrapwithYAAK_TARGET_ARCH: x64. Then it runs the release build's exact command,cargo test --all --exclude yaak-cli --features yaak-app-client/wry. I chose the full suite over just-p yaak-proxybecause this job exists to catch release-breaking test failures before a tag, and the full suite is what the release runs. The macOS job is unchanged.Windows is unverified: I couldn't run Windows from this Mac. The fix stands on reading the reqwest, native-tls and schannel sources plus the copied failure. It will only be proven once the PR's new Windows CI job runs.
Checks:
cargo test -p yaak-proxy --test proxy: 14 passed.pinned_client_falls_back_to_passthrough,certificate_alert_passes_through_on_the_next_connection,hang_up_after_the_handshake_passes_throughandabandoned_handshake_does_not_disable_capture: all 20 passed.cargo test --all --features yaak-app-client/wry: 884 passed, 0 failed. The first attempt failed to build only because this worktree hadn't been bootstrapped (vendored/protoc/includemissing). I ranvp installandnpm run bootstrapfirst, then reverted the build outputs bootstrap rewrote.cargo fmt -p yaak-proxy --checkandactionlint .github/workflows/ci.yml: both clean, and the YAML parses.codex review --uncommitted -c model_reasoning_effort=xhigh) found no actionable issues, so there was nothing to decline or re-run. It reviewed the uncommitted changes rather than--base main, because AGENTS.md forbids committing without approval.Written and run by Stetta.