Regression when connecting with a http::Request

[1c3t3a]

Hi there! From the latest version on I get a regression when connecting with a http::Request. In version 0.17.1 I get an WebsocketError(Protocol(InvalidHeader("sec-websocket-key"))) error when connecting via a Request, which worked back in earlier versions. Some example code to trigger the error (using tokio-tungstenite):

let mut req = http::Request::builder().uri(url.as_str());
let (ws_stream, _) = client_async(
            req.body(()).unwrap(),
            stream
        )
        .await?;

I looked into the issue and found out that the changes of PR #259 changed behavior for checking the key for a requests. In version 0.16.0 every request's sec-websocket-key header was set:

tungstenite-rs/src/handshake/client.rs

Lines 55 to 65 in 958fdb9

	let key = generate_key();
	let machine = {
	let req = generate_request(request, &key)?;
	HandshakeMachine::start_write(stream, req)
	};
	let client = {
	let accept_key = derive_accept_key(key.as_ref());
	ClientHandshake { verify_data: VerifyData { accept_key }, config, _marker: PhantomData }
	};

But the changes of PR #259 changed the behavior and now expect the sec-websocket-key header on every request:

tungstenite-rs/src/handshake/client.rs

Lines 97 to 122 in 45dad40

	fn generate_request(mut request: Request) -> Result<(Vec<u8>, String)> {
	let mut req = Vec::new();
	write!(
	req,
	"GET {path} {version:?}\r\n",
	path = request.uri().path_and_query().ok_or(Error::Url(UrlError::NoPathOrQuery))?.as_str(),
	version = request.version()
	)
	.unwrap();
	// Headers that must be present in a correct request.
	const KEY_HEADERNAME: &str = "Sec-WebSocket-Key";
	const WEBSOCKET_HEADERS: [&str; 5] =
	["Host", "Connection", "Upgrade", "Sec-WebSocket-Version", KEY_HEADERNAME];
	// We must extract a WebSocket key from a properly formed request or fail if it's not present.
	let key = request
	.headers()
	.get(KEY_HEADERNAME)
	.ok_or_else(|| {
	Error::Protocol(ProtocolError::InvalidHeader(
	HeaderName::from_bytes(KEY_HEADERNAME.as_bytes()).unwrap(),
	))
	})?
	.to_str()?
	.to_owned();

This is fine for passing in a Uri or a String as the IntoClientRequest implementation covers the conversion:

tungstenite-rs/src/client.rs

Lines 215 to 238 in 45dad40

	impl IntoClientRequest for Uri {
	fn into_client_request(self) -> Result<Request> {
	let authority = self.authority().ok_or(Error::Url(UrlError::NoHostName))?.as_str();
	let host = authority
	.find('@')
	.map(|idx| authority.split_at(idx + 1).1)
	.unwrap_or_else(|| authority);
	if host.is_empty() {
	return Err(Error::Url(UrlError::EmptyHostName));
	}
	let req = Request::builder()
	.method("GET")
	.header("Host", host)
	.header("Connection", "Upgrade")
	.header("Upgrade", "websocket")
	.header("Sec-WebSocket-Version", "13")
	.header("Sec-WebSocket-Key", generate_key())
	.uri(self)
	.body(())?;
	Ok(req)
	}
	}

But not for a http::Request. A possible solution would be to add the sec-websocket-key header in the implementation of IntoClientRequest, in case it is not yet present on a Request. Or is the intention that the user should provide it's own key when using a http::Request for connection?

[fredrik-jansson-se]

Having the same issue after upgrading.

[daniel-abramov]

The conclusions about change in logic that we introduced in 0.17.0 are correct 🙂

Essentially, if you're just simply using the tungstenite, e.g. you want to connect to an endpoint and use WebSockets, you could simply pass the URI and rely on the IntoClientRequest implementation that takes care of generating all required headers. But not when we receive http::Request, in which case we simply "pass-through" the request (after verifying that it's a correct request). Some rationale could be found here.

Or is the intention that the user should provide its key when using a http::Request for connection?

Depends on your use case. If there is no strong reason to create a custom http::Request, you could simply pass &str and benefit from tungstenite generating headers for you. In case you want to benefit from tungstenite generation of headers, but alter some of them, this could be a solution: #264 (comment)

Hope this helps 🙂