Jetty.project: Support empty HTTP header values

[pschorf]

As I understand it, RFC 7230 supports headers with empty values:

field-value = *( field-content / obs-fold )

But HttpGenerator has the comment

// rfc7230 does not allow no value

on line 605 which skips headers with no value. Am I misreading the spec? If not, would it be possible to update Jetty to support passing these headers?

[joakime]

An empty header field-value is not valid per RFC7230.

https://tools.ietf.org/html/rfc7230#section-3.2

The field-value is not optional and must be specified.

There is only 1 exception made for this rule, and it only applies to the Client and its declaration of the "Host" request header under some very specific conditions.

All server generated headers are never empty, as this is cause of many security issues (mostly with intermediaries).

Have you tried just sending an empty quoted string? ""

[pschorf]

I did some more research and after reading the errata I agree. I maintain a proxy so I'll need to investigate whether or not any clients actually use this feature but it came up as a regression.

Thanks for taking the time to reply.

[jamshid]

An empty header field-value is not valid per RFC7230.

According to the people that should know, that is not correct. Empty header values are valid http, see mnot's reply in https://github.com/whatwg/fetch/issues/332.

Breaking your users on upgrade is never a good idea. Please fix jetty to continue supporting empty headers.

[joakime]

@gregw you'll want to review this.

[joakime]

@jamshid btw, if you want RFC2616 behavior (where empty header values were allowed), you can configure your Jetty Connector to use RFC2616 behavior. But be aware that using empty headers with modern browsers can result in unexpected behavior, as they have universally adopted the updated RFCs (this isn't limited to RFC2616 vs RFC7230 btw, but all associated RFCs).

[gregw]

Having read RFC7230 again, specifically:

     field-value    = *( field-content / obs-fold )
     field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]

That's clear that an empty field-value is acceptable due to the *() rule. So I'm wondering exactly where we got the idea that it was not? I'm guessing at this stage it was just a miss-reading of the ABNF, which is a bit convoluted.... in fact it is a real mess... field-content can be a single character OR a single character followed by 1 or more space/tabs followed by another single character.

So a value like pigs can \t fly breaks down as:

• field-content p
• field-content i
• field-content g
• field-content s c
• field-content a
• field-content n \t f
• field-content l
• field-content y

No wonder we may have been confused!

I'll prepare a fix now, but will do a bit more research as well to truly understand if we did this for a reason or were just confused by horrid ABNF?

[gregw]

So HttpParser was not enforcing no empty headers, it only enforces a missing colon.
It was only HttpGenerator that was skipping headers with null or empty values.

It is a trivial fix to make the generator allow empty values, but currently I have it still skipping null values (because adding a null value to our HttpFields class is the same as removing that entry)

Note that the jetty compliance mode will not help in the HttpGenerator, as that appears to have been added without a check of the compliance mode anyway! Nor was there a unit test for that behaviour?!?! It was my commit - so my bad !!

I'll push the simple fix plus test to check it is fixed soon, but I think we need to check higher layers as well to make sure that empty and/or null values are being correctly handled.

@jamshid @pschorf do you have a particular use-case we can verify?

[joakime]

With this implemented, we need to investigate the following behavior changes...

HttpServletResponse:

• getHeader(String name) - this says "the value of the response header with the given name, or null if no header with the given name has been set on this response." - which means a null header value is returned, which has the meaning that it hasn't been set.
• containsHeader(String name) - this would return false with a null header value
• setHeader(String name, String value) - sets the header to null, but null means no header has been set per the javadocs.
• addHeader(String name, String value) - what does this do if the header was set to null prior to this? or what if it was setHeader("foo", "bar"); addHeader("foo", null); addHeader("foo", "baz");, what is the expected result?
• getHeaderNames() - since getHeader(String name) and containsHeader(String name) both indicate that null means not-set, should the headers with null value be returned here?
• getHeaders(String name) - this returns a collection representing the multi value header values, should this allow null entries?

We will probably need extra logic to forbid specific headers from ever being set as null, as there are some headers that are not allowed to be null (Content-Type, Content-Length, Connection, etc...)

HttpServletRequest:

• getDateHeader(String name) - this returns a long, what should we do when the header received is a null value? (probably the same thing we do if the value is bogus. eg: Date: xyz-pdq results in getDateHeader("Date") returning -1 meaning that the header wasn't set)
• getIntHeader(String name) - same thing as above.
• getHeader(String name) - this says "a String containing the value of the requested header, or null if the request does not have a header of that name" which means that returning null means that the request didn't have the header.
• getHeaders(String name) - this returns an Enumeration<String> of the header values (same question as HttpServletResponse on how to deal with null values. The servlet api says to return null if it isn't set, and a null value means not set.)

Perhaps null should just not be allowed in the API usages, but empty string should.
The HttpGenerator should treat HttpFields with null as non-existent, but HttpFields with empty string as empty values.

If this make sense, then wouldn't the HttpParser also need to be aware of this and set the HttpFields for no-value headers to empty string to keep things consistent?

Since we dropped HTTP/0.9 support I would guess that some of this gets easier at the HttpParser level (don't need to worry about strange multi value quirks like r or n delimited values)

Also, what does HTTP/2 say about empty header values?

[sbordet]

HTTP/2 _requires_ empty headers because a valid HTTP header for H2C is HTTP2-Settings:, ie the empty value.

[pschorf]

@gregw I don't recall what the original request that drove this was: I do want to make sure that the behavior for null stays the same, as we are using that for removing the automatically generated content-type headers. We have a web proxy which uses the Jetty HTTP client to connect to our backends, so it would have probably originally from an end-user. If you have a fix I can write up a quick test on our end to verify that it works.

[joakime]

Since there are no HttpServletResponse (and HttpServletRequest) methods for deleting or clearing headers, supporting setHeader("foo", null) to delete a header makes sense.

Both Glassfish and Tomcat honor this behavior as well - setHeader("foo", null) will remove the header if previously set.

However, using setHeader("foo", "") could (should?) result in an empty header.

[gregw]

@joakime exactly. setHeader("foo",null) removes the header. setHeader("foo","") sets an empty header.

So I think we are good.

[pliniosilveira]

After all, can you summarize what was and what are Jetty's behavior in each version?

If I understood

setHeader("foo",null) removes the header. setHeader("foo","") sets an empty header.

applies to jetty 9.4 and

setHeader("foo",null) or setHeader("foo","") both remove the header.

applies to jetty 9.3 or less. Is that correct?

[Mouvedia]

related: https://www.rfc-editor.org/errata/eid5249

[joakime]

related: https://www.rfc-editor.org/errata/eid5249

That is a Rejected Errata for HTTP/2