Empty headers panic thread

[bryanburgers]

While testing for #1696, I wanted to test what happens when

1. A User-Agent: test/1 is sent
2. No User-Agent header is sent
3. An empty User-Agent: header is sent.

1 and 2 seem to check out as expected. Oddly, case 3 causes a 500 internal error because of a thread panic. It seems to occur for any header.

bryan ~/projects/cratesio/ pr/1696 $ curl -i --head localhost:8888/api/v1/crates/bryan -H "User-Agent;"
HTTP/1.1 500 Internal Server Error
HTTP/1.1 404 Not Found
Content-Length: 30
Date: Fri, 05 Apr 2019 19:08:55 GMT
Connection: keep-alive

bryan ~/projects/cratesio/ pr/1696 $ curl -i --head localhost:8888/api/v1/crates/bryan -H "X-Anything;"
HTTP/1.1 500 Internal Server Error
HTTP/1.1 404 Not Found
Content-Length: 30
Date: Fri, 05 Apr 2019 19:09:04 GMT
Connection: keep-alive

Output from the web server is:

thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
  version: 1.1.0
  method: Head
  scheme: Http
  host: Name("localhost:8888")
  path: /api/v1/crates/bryan
  query_string: None
  remote_addr: V4(127.0.0.1:53322)

It's likely this is in a dependency, and it's probably unimportant. But noting it here anyway.

Note: the semi-colon in the command line invocation of curl looks funny. But it's the official way to send an empty header. From the man-page:

-H, --header

(HTTP) Extra header to include in the request when sending HTTP to a server. You may specify any number of extra headers. Note that if you should add a custom header that has the same name as one of the internal ones curl would use, your externally set header will be used instead of the internal one. This allows you to make even trickier stuff than curl would normally do. You should not replace internally set headers without knowing perfectly well what you're doing. Remove an internal header by giving a replacement without content on the right side of the colon, as in: -H "Host:". If you send the custom header with no-value then its header must be terminated with a semicolon, such as -H "X-Custom-Header;" to send "X-Custom-Header:".

[sgrif]

This appears to only happen in development mode (if you send the same request to production it'll work fine). It's coming from the debug middleware when it attempts to print the headers. The iterator used by Headers::all in rust-civet makes this assumption (https://github.com/wycats/rust-civet/blob/master/src/lib.rs#L223), but Headers::find does not. Interestingly rust-civet doesn't actually treat a header with no value any differently than the header not being present, it'll return None in this case.

We've been moving away from civet in favor of hyper (which you can test locally by setting USE_HYPER=1), which appears to have the same bug in Headers::all (https://github.com/jtgeibel/conduit-hyper/blob/master/src/lib.rs#L53), but does the correct thing for Headers::find (https://github.com/jtgeibel/conduit-hyper/blob/master/src/lib.rs#L32).

We should probably fix this in one or both of those, but it's pretty low priority. Other than this debug middleware, we only look up individual headers, and we just return an empty string if we didn't find it (which unintentionally normalizes the behavior between these two servers)

crates.io/src/util/request_helpers.rs

Lines 13 to 17 in e2f20c7

	pub fn request_header<'a>(req: &'a dyn Request, header_name: &str) -> &'a str {
	req.headers()
	.find(header_name)
	.and_then(|x| x.first().cloned())
	.unwrap_or_default()

Note that none of this affects tests, which don't actually hit the web server at all.