HeaderName::from_static requires all-lowercase HTTP2 compatible hea…

[sdroege]

…der names

and was passed header names with uppercase characters instead, which
made it panic.

[sdroege]

See https://docs.rs/http/latest/http/header/struct.HeaderName.html#method.from_static

[sdroege]

Also it's unclear to me why none of the existing tests caught this.

[sdroege]

This was introduced in d661f57 CC @application-developer-DA

[daniel-abramov]

Oh, we missed this case somehow, thanks @sdroege.

Also it's unclear to me why none of the existing tests caught this.

It seems like this particular panic may only happen when an invalid Request passed to tungstenite, e.g. when the user did not use the into_client_request() for the &str to generate the Request and alter it afterward, but instead the user did generate the Request by hand and passed to tungsetenite. Previous versions modified the request from the user, altering the headers, etc (to match the RFC) and generating all required values. However, it has been reported a couple of times that such behavior is not very obvious because some people seem to want to use their own headers (Host) that they set to some other value and did not expect tungstenite overwriting it. So after d661f57 we have a new logic that only generates the request and missing fields if the user lets tungstenite do so (e.g. by passing a string to the connect or any other means that would call the IntoClientRequest trait implementation for the Uri). However, if the user creates the Request by hand and passes it to tungstenite, tungstenite no longer alters/writes headers to it and instead just passes it through (verifying that all required headers are present). I think we have not covered this case with unit tests. Let me write the tests for it.

[daniel-abramov]

Should we instead just call to_lowercase()? Would look a bit more elegant (and less duplication of code), but has a greater runtime cost. However, that runtime cost is probably negligible since it only kicks in when the user passes the poorly formed Request to tungstenite (e.g. most users that just use connect are unlikely to be affected by this runtime penalty).

[sdroege]

That would be the other option. I can update the PR accordingly, your decision which variant you prefer :)

[sdroege]

Well, calling HeaderName::from_bytes().

That's btw called implicitly by the remove() and get() calls on the strings too and does heap allocations. If that's something you worry about then the list of header names should be stored as actual HeaderNames instead of strings statically (e.g. via once_cell::sync::Lazy).

[daniel-abramov]

That would be the other option. I can update the PR accordingly, your decision which variant you prefer :)

I think I would prefer the option with to_lowercase() :) It's a bit more readable (arguably easier to maintain like this) and the runtime cost would only be present when invalid requests are passed, so I think for 95% of cases we're good 👍

[sdroege]

Sure, updated accordingly

[daniel-abramov]

Thanks! 👍

[sdroege]

Can you release a 0.17.2 with this fix in the near future?

[daniel-abramov]

Sure! I'll try to publish it today along with the updated tokio-tungstenite.

(bugs like this we try to update and push fast)

[sdroege]

Nice, thanks! I noticed this btw from a user of async-tungstenite. I've released a version of that using tungstenite 0.17 during the weekend.