Issue #12104 - HTTP/1.0 should produce a valid `Connection` header, never blank/null. #12105

joakime · 2024-07-29T21:18:49Z

Enable ee9 ErrorHandlerTest again
Fix empty Connection: response header issue seen on HTTP/1.0

Fixes #12104

* make it easier to test different scenarios

+ Fix empty `Connection:` response header issue seen on HTTP/1.0

header issue seen on HTTP/1.0

joakime · 2024-07-30T16:49:59Z

Setting this to draft mode, as there's still a problem with the response headers on HTTP/1.0 to address.

header issue seen on HTTP/1.0 when Servlet uses `Connection: keep-alive` against a request that wasn't persistent.

…9-errorhandlertest-restore

joakime · 2024-07-31T14:26:35Z

@gregw this started out being a fix for HTTP/1.0 responses sometimes having an empty Connection: header.
But its growing in ways I'm uncomfortable with.
I'm going tools down on this PR until we can discuss it, in detail.
There's no rush for this PR either.

gregw · 2024-08-01T08:26:16Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java

+                            boolean hasValue = StringUtil.isNotBlank(field.getValue());
+                            boolean hasKeepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString());
+                            boolean hasClose = field.contains(HttpHeaderValue.CLOSE.asString());


Whilst this style is easily readable, it contains a lot of redundant code/tests. If hasValue==false, then you can break straight away and avoid subsequent work.

If it hasKeepAlive is true, then in your code there is no need to lookup hasClose (but see below).

gregw · 2024-08-01T08:27:40Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java

                            }
-                            if (field.contains(HttpHeaderValue.CLOSE.asString()))
+                            else if (hasClose)


I'm not sure we want the else here. If it came Connection: keep-alive, close then either that is a 400 or at least we should close. So perhaps invert the order of the tests and look for close, else look for keepAlive.

I was reading the Hop-by-hop parts of the spec in light of other tests with have for this.

Looks like if we receive a comma delimited list, then it's a 400 Bad Request in normal operations.
BUT if we receive that when we are a proxy, then we have to act on all of the various Hop-by-hop headers (of which Connection is just one of them).

BUT we can create a response with multi-value Connection at all times (normal operation, and proxy operation), and we are only on the hook to act on a persistence state change for our hop.

I'm at a loss on the exact meaning of "our hop" in these cases.
Eg: If the request was keep-alive and response from the servlet is close, keep-alive vs keep-alive, close, what do we do at the prepareRequest / httpGenerator?

I don't think we need to 400 for both a Keep-Alive and Close. The close is a must-not persist and the Keep-Alive is a may persist, so they combine easily to a must-not persist.

gregw · 2024-08-01T08:29:24Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

+            boolean hasConnectionClose = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString());
+            boolean hasConnectionKeepAlive = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.toString());


again, don't lookup values if we don't need them, especially in this case as it involves a full iteration to do each lookup.

So lookup only if needed and perhaps look for close first, so it can override keepalive.

The problem is that each of these could then be looked up multiple times instead.
So I took the case of the lookup occurring 4 times down to 2, which I felt was better.

A single iteration through is best, I'll work on that after you have had a look at the ResponseHeadersTest.

In particular the new tests ...

testHTTP10ConnectionBehavior for when the client is using HTTP/1.0

testHTTP11ConnectionBehavior for when the client is using HTTP/1.1

Are you good with the set of expectations for those?
Each test case arguments have 3 parameters.

The Connection request header as sent from the Client (null means no Connection header sent)

The Connection response header as set by an application (Servlet in this case. null means no Connection header is set)

The expected Connection response header as seen by the client. (null means no Connection header should be present received)

Once I get that set of tests sane, then I can look closer at the other tests that are now failing due to this PR.
Some of which are suspect (like a test client that uses HTTP/1.0 but also Connection: close, which is not typical, but we should accept it, and the new testcases I mentioned has variants of this scenario too)

If you take just the changes to the ResponseHeadersTest back to jetty-12.0.x HEAD and run them, you'll quickly see the various cases that currently return a Connection: (empty value) response header.

I'm not saying avoid local variables with those looked up values (so we don't need to lookup multiple times), but I am saying delay doing the lookup until it is known that it is needed.

gregw · 2024-08-01T08:33:07Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java

+                                String resultingValue = Stream.of(field.getValues())
+                                    .filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
+                                    .collect(Collectors.joining(", "));


I wonder if string operations here would be better than streams...
Also perhaps we could do this in the condition bodies above?

Yeah, I'm not a fan of the stream operation here, but it was here before I started this, so I left it.
I was going to look into adding a HttpField.removeValue(String) method as a possible replacement.

Yeah, so that. I agree a removeValue method would allow us to hide the details.

joakime · 2024-08-01T13:10:24Z

@gregw the branch jetty-12.0.x-null-connection-response-header has just the changes in ResponseHeadersTest.java (commit 11e8faf)

Build results -> https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/activity?branch=jetty-12.0.x-null-connection-response-header

Basically, this is the state of jetty-12.0.x HEAD at the moment.
I really need a first pass through the test cases / expectations, as I'm sure some of the expectations are not right.
But I'm 100% sure that sending Connection: (a blank value of "") is wrong.

testHTTP10ConnectionBehavior(String, String, String)

Num	client-req	servlet-resp	expected-client-resp	pass / fail
1	null	null	close	❌ (Connection value was null)
2	null	close	close	✅
3	null	keep-alive	close	❌ (Connection value was "")
4	keep-alive	null	keep-alive	✅
5	keep-alive	close	close	✅
6	keep-alive	keep-alive	keep-alive	✅
7	close	null	close	❌ (Connection value was null)
8	close	close	close	✅
9	close	keep-alive	close	❌ (Connection value was "")
10	null	close, keep-alive	close	✅
11	null	keep-alive, close	close	✅
12	close	close, keep-alive	close	✅
13	close	keep-alive, close	close	✅
14	keep-alive	close, keep-alive	close, keep-alive	❌ (Connection value was "close")
15	keep-alive	keep-alive, close	keep-alive, close	❌ (Connection value was "close")

testHTTP11ConnectionBehavior(String, String, String)

Num	client-req	servlet-resp	expected-client-resp	pass / fail
1	null	null	null	✅
2	null	close	close	✅
3	null	keep-alive	null	❌ (Connection value was "keep-alive")
4	close	null	close	✅
5	close	close	close	✅
6	close	keep-alive	close	❌ (Connection value was "")
7	keep-alive	null	null	✅
8	keep-alive	close	close	✅
9	keep-alive	keep-alive	null	❌ (Connection value was "keep-alive")
10	null	close, keep-alive	close	✅
11	null	keep-alive, close	close	✅
12	close	close, keep-alive	close	✅
13	close	keep-alive, close	close	✅
14	keep-alive	close, keep-alive	close	✅
15	keep-alive	keep-alive, close	close	✅

gregw · 2024-08-02T02:28:05Z

@joakime I definitely agree that sending a blank connection is wrong, but I think I see a pretty simple fix for that.
I also see a bunch of other little cleanups that I'd like to do now that I've seen them. But ultimately, I don't think we need big changes.

Let me have a play with your branch with just the test changes and see if I can come up with a minimal fix... stand by...

gregw · 2024-08-02T02:30:54Z

Actually, I'm not sure why you have done this as eeX tests. I think this is a core issue??? I may move your test to core....

joakime · 2024-08-02T02:32:53Z

Actually, I'm not sure why you have done this as eeX tests. I think this is a core issue??? I may move your test to core....

It was originally discovered while testing ee8 issues reported from the community.
So I started there.

But now that I've been deep in it, I don't see this being a EE8/EE9/EE10 specific issue.

joakime · 2024-08-02T02:34:17Z

OH! And when I enabled the disabled test ErrorHandlerTest.java (in ee8/ee9) one of those tests also showed the problem.

gregw · 2024-08-02T04:30:10Z

See my fix on #12127. I moved your test to core and combined. I changed a few expected values. The fix is in the HttpGenerator and is a reworking of Connection handling that is a lot more explicit.

joakime · 2024-08-02T12:15:43Z

Closing in favor of alternate approach in #12127

joakime added 4 commits July 29, 2024 15:32

Modernize ResponseHeadersTest

4dd04ec

* make it easier to test different scenarios

Add ResponseHeadersTest for HTTP/1.0 Connection header

ba27531

Remove bad comment

e755458

Fixes #12104 - Enable ee9 ErrorHandlerTest again

88b145d

+ Fix empty `Connection:` response header issue seen on HTTP/1.0

joakime self-assigned this Jul 29, 2024

joakime requested a review from sbordet July 29, 2024 21:19

joakime added the Bug For general bugs on Jetty side label Jul 29, 2024

+ Fix empty Connection: response

d73e1a0

header issue seen on HTTP/1.0

joakime marked this pull request as draft July 30, 2024 16:49

joakime added 2 commits July 30, 2024 12:09

+ Fix empty Connection: response

e2b9423

header issue seen on HTTP/1.0 when Servlet uses `Connection: keep-alive` against a request that wasn't persistent.

More testing of other HTTP/1.0 and HTTP/1.1 connection header scenarios

ec489b0

joakime requested a review from gregw July 31, 2024 11:45

joakime added 2 commits July 31, 2024 08:01

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/ee…

01f4e64

…9-errorhandlertest-restore

Fixing testcase setup

040c046

gregw requested changes Aug 1, 2024

View reviewed changes

gregw reviewed Aug 1, 2024

View reviewed changes

joakime closed this Aug 2, 2024

joakime deleted the fix/12.0.x/ee9-errorhandlertest-restore branch August 2, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #12104 - HTTP/1.0 should produce a valid `Connection` header, never blank/null. #12105

Issue #12104 - HTTP/1.0 should produce a valid `Connection` header, never blank/null. #12105

joakime commented Jul 29, 2024 •

edited

Loading

joakime commented Jul 30, 2024

joakime commented Jul 31, 2024

gregw Aug 1, 2024

gregw Aug 1, 2024

joakime Aug 1, 2024 •

edited

Loading

gregw Aug 2, 2024

gregw Aug 1, 2024

joakime Aug 1, 2024

joakime Aug 1, 2024 •

edited

Loading

joakime Aug 1, 2024

gregw Aug 2, 2024

gregw Aug 1, 2024

joakime Aug 1, 2024

gregw Aug 2, 2024

joakime commented Aug 1, 2024 •

edited

Loading

gregw commented Aug 2, 2024

gregw commented Aug 2, 2024

joakime commented Aug 2, 2024

joakime commented Aug 2, 2024 •

edited

Loading

gregw commented Aug 2, 2024

joakime commented Aug 2, 2024

		boolean hasConnectionClose = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString());
		boolean hasConnectionKeepAlive = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.toString());

Issue #12104 - HTTP/1.0 should produce a valid Connection header, never blank/null. #12105

Issue #12104 - HTTP/1.0 should produce a valid Connection header, never blank/null. #12105

Conversation

joakime commented Jul 29, 2024 • edited Loading

joakime commented Jul 30, 2024

joakime commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime commented Aug 1, 2024 • edited Loading

gregw commented Aug 2, 2024

gregw commented Aug 2, 2024

joakime commented Aug 2, 2024

joakime commented Aug 2, 2024 • edited Loading

gregw commented Aug 2, 2024

joakime commented Aug 2, 2024

Issue #12104 - HTTP/1.0 should produce a valid `Connection` header, never blank/null. #12105

Issue #12104 - HTTP/1.0 should produce a valid `Connection` header, never blank/null. #12105

joakime commented Jul 29, 2024 •

edited

Loading

joakime Aug 1, 2024 •

edited

Loading

joakime Aug 1, 2024 •

edited

Loading

joakime commented Aug 1, 2024 •

edited

Loading

joakime commented Aug 2, 2024 •

edited

Loading