Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Curl does not lowercase header names for HTTP/2 and HTTP/3 #4400

Closed
tunetheweb opened this issue Sep 22, 2019 · 5 comments
Closed

Curl does not lowercase header names for HTTP/2 and HTTP/3 #4400

tunetheweb opened this issue Sep 22, 2019 · 5 comments
Labels
HTTP/2 HTTP/3 h3 or quic related

Comments

@tunetheweb
Copy link
Contributor

Curl does not currently lower case HTTP header names as required by HTTP/2 and HTTP/3. The lower-level libraries that curl uses (nghttp for HTTP/2 and ngtcp2/quiche for HTTP/3) do lower case the header names, but it does mean for some of the output, before the lower library is invoked, curl can show upper case header names which could be confusing.

While this may seem somewhat pedantic, I still see some developers who are caught by surprised by this more strict requirement for HTTP/2 and HTTP/3 so would prefer curl to not display this as it does currently.

For example:
HTTP/3 output with incorrect HTTP header names

As I see it, we can do one of three things here:

  1. Ignore it. It's just a display issue and at the end of the day curl behaves correctly when it goes to actually send the message. Lowercasing the header names may have a slight performance cost and it will be done anyway in lower level library so pointless doing it twice. We have bigger fish to fry so let's close this issue.
  2. Correct this when printing the output for HTTP/2 and HTTP/3, which is relatively low risk as not really changing anything except the output.
  3. Correct the actual header name when using HTTP/2 or HTTP/3, which does alter the actual header name, so may be higher risk than 2, but which the lower level library should be doing anyway so maybe not that much of a risk.

I've taken a stab at option 3 here: master...bazzadp:lowercase_h2_h3_headernames.

I'd appreciate your thoughts on this and if it's worth submitting this as a pull request?

It's my first potential commit to curl, and my C is a little rusty, so please be kind :-)
@bagder
Copy link
Member

bagder commented Sep 22, 2019

Thanks @bazzadp! I think it looks perfectly reasonable so please make a PR out of it!

@bagder bagder added HTTP/2 HTTP/3 h3 or quic related labels Sep 22, 2019
@tunetheweb tunetheweb mentioned this issue Sep 22, 2019
Closed
@jay
Copy link
Member

jay commented Sep 23, 2019

putting aside the lowercase question why are the headers prefixed h3 like h3 [key: val] and not just key: val

@bagder
Copy link
Member

bagder commented Sep 23, 2019

That h3 output is debug-only output that a normal build won't show:

curl/lib/vquic/quiche.c

Lines 697 to 699 in 41db01a

H3BUGF(infof(data, "h3 [%.*s: %.*s]\n",
nva[i].name_len, nva[i].name,
nva[i].value_len, nva[i].value));

I would imagine we can remove that at some point as it doesn't really add much.

@tunetheweb
Copy link
Contributor Author

tunetheweb commented Sep 23, 2019 •
edited

It's the same with h2 [key: val], the only difference being at the moment h2 is turned off by default, where as h3 DEBUG_HTTP3 is currently turned on for Quiche:

curl/lib/vquic/quiche.c

Lines 43 to 46 in 41db01a

#define DEBUG_HTTP3
/* #define DEBUG_QUICHE */
#ifdef DEBUG_HTTP3
#define H3BUGF(x) x

A fact, incidentally, that caused me some confusion when I initially tried to test the HTTP/2 part of this change, as didn't realise I had to explicitly turn on DEBUG_HTTP2 in the code to see similar output for h2 :-)

Anyway, because I went with my option 3, this change also changes the non-debug lines below this (except for the Host: hostname line which is a dummy HTTP/1.1-style line which is created by curl):

> HEAD / HTTP/2
> Host: www.tunetheweb.com
> user-agent: curl/7.67.0-dev
> accept: */*

as opposed to:

> HEAD / HTTP/2
> Host: www.tunetheweb.com
> User-Agent: curl/7.67.0-dev
> Accept: */*

So there is still value in this change, even when not in debug mode, and even when h3 changes to act more like h2.

@bagder
Copy link
Member

bagder commented Sep 23, 2019 •
edited

We should probably at least make the DEBUG_HTTP3 like this, if not disabled completely by default:

#ifdef CURLDEBUG
#define DEBUG_HTTP3
#endif

The HTTP/2 debug is completely disabled by default pretty much because it generates so much data that is mostly uninteresting for everyone as long as you don't actually debug HTTP/2 related issues.

@bagder bagder closed this as completed in 0023fce Sep 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
HTTP/2 HTTP/3 h3 or quic related
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Lowercase headernames for HTTP/2 and HTTP/3 tunetheweb/curl
3 participants
@bagder @jay @tunetheweb