fix(http)!: honor custom configuration with safe WebSocket handshakes - #333
Open
SebTardif wants to merge 2 commits into
Open
fix(http)!: honor custom configuration with safe WebSocket handshakes#333SebTardif wants to merge 2 commits into
SebTardif wants to merge 2 commits into
Conversation
run_ws discarded the configured Client and called async-tungstenite connect_async with only the URL. HttpClient::with_client and with_endpoint_and_client therefore ignored timeout, default headers, proxy, and TLS on ws:// and wss://. Perform the handshake with the reqwest Client, then wrap the upgraded stream as a tungstenite WebSocket. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Build configured transports through HttpClientBuilder so WebSocket handshakes use HTTP/1.1 and cannot follow redirects. Validate the complete response before sending ACP data while preserving HTTP/SSE configuration and reusable clients. Keep with_client and with_endpoint_and_client as deprecated HTTP/SSE wrappers. Cover TLS ALPN, proxies, redirects, timeouts, malformed handshakes, feature unification, and migration behavior. BREAKING CHANGE: WebSocket URLs passed to constructors taking a prebuilt reqwest Client now return WebSocketRequiresBuilder before network I/O. Use builder or builder_with_endpoint with configure_http instead.
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.
Summary
The WebSocket path previously discarded the configured reqwest client and called tungstenite with only the URL. This change uses reqwest for the opening handshake so headers, timeouts, proxies, DNS, and TLS configuration are honored, then uses the existing async-tungstenite stack for framing.
Client construction now goes through an SDK builder so WebSocket connection policies can be applied before the underlying reqwest client is built:
builder(base_url)retains the/acpnormalization rule;builder_with_endpoint(endpoint)preserves an exact endpoint.from_http_client(exact_endpoint, client)supports reuse of an existing HTTP/SSE client and its connection pool.newandwith_endpointremain available unchanged;HttpClientcan be cloned to share its underlying HTTP client.Compatibility and migration
BREAKING CHANGE: WebSocket URLs passed to constructors taking an already-built reqwest client now return
HttpClientError::WebSocketRequiresBuilderbefore network I/O. An opaque, prebuilt client cannot be reconfigured to enforce the WebSocket connection policies. Use the builder API above instead.with_clientandwith_endpoint_and_clientare retained as deprecated compatibility wrappers, not removed. Existing HTTP/SSE calls preserve their path handling, configured client, and connection pool. They can migrate incrementally to the builder orfrom_http_client; the latter takes an exact endpoint and does not append/acp.Normal reqwest TLS options support custom trust roots and client identities. A raw
tls_backend_preconfiguredbackend must itself select HTTP/1.1 ALPN: reqwest does not rewrite preconfigured TLS ALPN. Incompatible negotiation fails before ACP data is sent.Request/read timeouts cover the opening WebSocket handshake, not the lifetime of the upgraded socket. HTTP/SSE keeps normal reqwest request/body timeout semantics.
The book, rustdoc, README, and changelog include configuration and migration guidance.
Verification
just test— full workspace suite passed.cargo fmt --all -- --check— passed.-D warnings— passed.mdbook buildand HTTP crate rustdoc with-D warnings— passed.Regression coverage includes malformed handshakes with zero ACP writes, deprecated-constructor migration behavior, default headers, separate request/read handshake timeouts, custom trust roots and HTTP/2 feature unification, raw TLS ALPN compatibility, redirects that never reach their destination (including WSS-to-HTTP downgrade), authenticated CONNECT proxy tunnels, upgraded-stream timeout lifetime, and preservation of HTTP/SSE redirect and HTTP/2 policies. TLS certificates are generated in memory at test runtime; no private key fixtures are committed.