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.

Sign up for GitHub

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

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4dd04ec
Modernize ResponseHeadersTest
joakime Jul 29, 2024
ba27531
Add ResponseHeadersTest for HTTP/1.0 Connection header
joakime Jul 29, 2024
e755458
Remove bad comment
joakime Jul 29, 2024
88b145d
Fixes #12104 - Enable ee9 ErrorHandlerTest again
joakime Jul 29, 2024
d73e1a0
+ Fix empty `Connection:` response
joakime Jul 30, 2024
e2b9423
+ Fix empty `Connection:` response
joakime Jul 30, 2024
ec489b0
More testing of other HTTP/1.0 and HTTP/1.1 connection header scenarios
joakime Jul 30, 2024
01f4e64
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/ee…
joakime Jul 31, 2024
040c046
Fixing testcase setup
joakime Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,7 @@ private void generateHeaders(ByteBuffer header, ByteBuffer content, boolean last

// default field values
HttpField transferEncoding = null;
boolean http10 = _info.getHttpVersion() == HttpVersion.HTTP_1_0;
boolean http11 = _info.getHttpVersion() == HttpVersion.HTTP_1_1;
boolean close = false;
boolean chunkedHint = _info.getTrailersSupplier() != null;
Expand Down Expand Up @@ -627,23 +628,36 @@ else if (contentLength != field.getLongValue())

case CONNECTION:
{
boolean keepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString());
if (keepAlive && _info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null)
boolean hasValue = StringUtil.isNotBlank(field.getValue());
boolean hasKeepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString());
boolean hasClose = field.contains(HttpHeaderValue.CLOSE.asString());
Comment on lines +631 to +633
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


if (http10 && hasKeepAlive)
{
_persistent = true;
// set persistence if unset
if (_persistent == null)
_persistent = true;
}
if (field.contains(HttpHeaderValue.CLOSE.asString()))
else if (hasClose)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@joakime joakime Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
close = true;
_persistent = false;
}
if (keepAlive && _persistent == Boolean.FALSE)

if (_persistent == Boolean.FALSE && hasKeepAlive)
{
field = new HttpField(HttpHeader.CONNECTION,
Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
.collect(Collectors.joining(", ")));
// we need to strip the keep-alive values
String resultingValue = Stream.of(field.getValues())
.filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
.collect(Collectors.joining(", "));
Comment on lines +650 to +652
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (StringUtil.isBlank(resultingValue))
hasValue = false; // all values stripped, do not produce a Connection header with no values
else
field = new HttpField(HttpHeader.CONNECTION, resultingValue);
}
putTo(field, header);

if (hasValue)
putTo(field, header);
break;
}

Expand Down
1 change: 1 addition & 0 deletions jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,7 @@ public void testConnectionKeepAliveWithAdditionalCustomValue() throws Exception
public void testKeepAliveWithClose() throws Exception
{
HttpGenerator generator = new HttpGenerator();
generator.setPersistent(false);
HttpFields.Mutable fields = HttpFields.build();
fields.put(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.asString() + ", other, " + HttpHeaderValue.CLOSE.asString());
MetaData.Response info = new MetaData.Response(200, "OK", HttpVersion.HTTP_1_0, fields);
Expand Down
55 changes: 52 additions & 3 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.atomic.LongAdder;
import java.util.stream.Collectors;

import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.ComplianceViolation;
Expand Down Expand Up @@ -1382,10 +1383,58 @@ public void demand()
}

@Override
public void prepareResponse(HttpFields.Mutable headers)
public void prepareResponse(HttpFields.Mutable responseHeaders)
{
if (_connectionKeepAlive && _version == HttpVersion.HTTP_1_0 && !headers.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()))
headers.add(HttpFields.CONNECTION_KEEPALIVE);
boolean hasConnectionClose = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString());
boolean hasConnectionKeepAlive = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.toString());
Comment on lines +1388 to +1389
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@joakime joakime Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. The Connection request header as sent from the Client (null means no Connection header sent)
  2. The Connection response header as set by an application (Servlet in this case. null means no Connection header is set)
  3. 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (_version == HttpVersion.HTTP_1_0 && _generator.isPersistent() && _connectionKeepAlive && !hasConnectionClose)
{
// attempt to cover HTTP/1.0 case where the Connection response header
// doesn't have a close value (set by the application) and it needs to
// be represented as a `Connection: keep-alive` response instead due
// to the existence of the `Connection: keep-alive` request header
responseHeaders.add(HttpFields.CONNECTION_KEEPALIVE);
}

if (!_generator.isPersistent())
{
if (!hasConnectionClose)
{
// Add missing "Connection: close" if not persistent
responseHeaders.add(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE);
}

if (hasConnectionKeepAlive)
{
// if generator is not persistent, strip any keep-alive
// response header values set by the application regardless of
// HTTP/1.0 and HTTP/1.1 differences
String resultingValue = responseHeaders.getValuesList(HttpHeader.CONNECTION)
.stream()
.filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
.collect(Collectors.joining(", "));
if (StringUtil.isBlank(resultingValue))
responseHeaders.remove(HttpHeader.CONNECTION);
else
responseHeaders.put(HttpHeader.CONNECTION, resultingValue);
}
}
else
{
// Strip "keep-alive" when using HTTP/1.1 on persistent connections
if (_version == HttpVersion.HTTP_1_1 && hasConnectionKeepAlive)
{
String resultingValue = responseHeaders.getValuesList(HttpHeader.CONNECTION)
.stream()
.filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))
.collect(Collectors.joining(", "));
if (StringUtil.isBlank(resultingValue))
responseHeaders.remove(HttpHeader.CONNECTION);
else
responseHeaders.put(HttpHeader.CONNECTION, resultingValue);
}
}
}

@Override
Expand Down
1 change: 0 additions & 1 deletion jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,6 @@ public void ensureConsumeAllOrNotPersistent()
if (!HttpField.contains(coalesced, HttpHeaderValue.CLOSE.asString()))
coalesced += ", close";

// Returns a single Cookie header with all cookies.
return new HttpField(HttpHeader.CONNECTION, coalesced);

});
Expand Down
77 changes: 38 additions & 39 deletions jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.logging.StacklessLogging;
import org.eclipse.jetty.server.LocalConnector;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.ajax.JSON;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
Expand All @@ -55,7 +55,6 @@
import static org.hamcrest.Matchers.nullValue;
import static org.junit.jupiter.api.Assertions.assertTrue;

@Disabled // TODO
public class ErrorHandlerTest
{
StacklessLogging stacklessLogging;
Expand Down Expand Up @@ -281,22 +280,26 @@ public void test404PostHttp11() throws Exception
@Test
public void test404PostCantConsumeHttp10() throws Exception
{
String rawResponse = connector.getResponse(
"POST / HTTP/1.0\r\n" +
"Host: Localhost\r\n" +
"Accept: text/html\r\n" +
"Content-Length: 100\r\n" +
"Connection: keep-alive\r\n" +
"\r\n" +
"0123456789");

String rawRequest = """
POST / HTTP/1.0\r
Host: Localhost\r
Accept: text/html\r
Content-Length: 100\r
Connection: keep-alive\r
\r
0123456789
""";

String rawResponse = connector.getResponse(rawRequest);
HttpTester.Response response = HttpTester.parseResponse(rawResponse);

dump(response);

assertThat(response.getStatus(), is(404));
assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
assertThat(response.getContent(), containsString("content=\"text/html;charset=ISO-8859-1\""));
assertThat(response.getField(HttpHeader.CONNECTION), nullValue());
assertThat(response.get(HttpHeader.CONNECTION), is("close"));
assertContent(response);
}

Expand All @@ -308,7 +311,6 @@ public void test404PostCantConsumeHttp11() throws Exception
"Host: Localhost\r\n" +
"Accept: text/html\r\n" +
"Content-Length: 100\r\n" +
"Connection: keep-alive\r\n" + // This is not need by HTTP/1.1 but sometimes sent anyway
"\r\n" +
"0123456789");

Expand Down Expand Up @@ -703,7 +705,7 @@ public void testErrorContextRecycle() throws Exception
context.setErrorHandler(new ErrorHandler()
{
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
{
baseRequest.setHandled(true);
response.getOutputStream().println("Context Error");
Expand All @@ -717,35 +719,32 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
response.sendError(444);
}
});

context.setErrorHandler(new ErrorHandler()
server.setErrorHandler((request, response, callback) ->
{
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
baseRequest.setHandled(true);
response.getOutputStream().println("Server Error");
}
Content.Sink.write(response, true, "Server Error", callback);
return true;
});

server.start();

LocalConnector.LocalEndPoint connection = connector.connect();
connection.addInputAndExecute(BufferUtil.toBuffer(
"GET /foo/test HTTP/1.1\r\n" +
"Host: Localhost\r\n" +
"\r\n"));
String response = connection.getResponse();

assertThat(response, containsString("HTTP/1.1 444 444"));
assertThat(response, containsString("Context Error"));

connection.addInputAndExecute(BufferUtil.toBuffer(
"GET /test HTTP/1.1\r\n" +
"Host: Localhost\r\n" +
"\r\n"));
response = connection.getResponse();
assertThat(response, containsString("HTTP/1.1 404 Not Found"));
assertThat(response, containsString("Server Error"));
try (LocalConnector.LocalEndPoint connection = connector.connect())
{
connection.addInputAndExecute(BufferUtil.toBuffer(
"GET /foo/test HTTP/1.1\r\n" +
"Host: Localhost\r\n" +
"\r\n"));
String response = connection.getResponse();

assertThat(response, containsString("HTTP/1.1 444 444"));
assertThat(response, containsString("Context Error"));

connection.addInputAndExecute(BufferUtil.toBuffer(
"GET /test HTTP/1.1\r\n" +
"Host: Localhost\r\n" +
"\r\n"));
response = connection.getResponse();
assertThat(response, containsString("HTTP/1.1 404 Not Found"));
assertThat(response, containsString("Server Error"));
}
}
}
Loading
Loading