Skip to content

Commit

Permalink
Fixes #5152 - HttpClient should handle unsolicited responses.
Browse files Browse the repository at this point in the history
Now closing the connection if an unsolicited response is detected,
no matter what response status code, or whether it has a
Connection: close header, or whether it's just random bytes from
the server, and also no matter whether the client read -1.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
  • Loading branch information
sbordet committed Aug 18, 2020
1 parent 0646e4d commit c88aba6
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res
private RetainableByteBuffer networkBuffer;
private boolean shutdown;
private boolean complete;
private boolean unsolicited;

public HttpReceiverOverHTTP(HttpChannelOverHTTP channel)
{
Expand Down Expand Up @@ -264,6 +265,7 @@ public int getHeaderCacheSize()
public boolean startResponse(HttpVersion version, int status, String reason)
{
HttpExchange exchange = getHttpExchange();
unsolicited = exchange == null;
if (exchange == null)
return false;

Expand All @@ -279,7 +281,8 @@ public boolean startResponse(HttpVersion version, int status, String reason)
public void parsedHeader(HttpField field)
{
HttpExchange exchange = getHttpExchange();
if (exchange == null)
unsolicited |= exchange == null;
if (unsolicited)
return;

responseHeader(exchange, field);
Expand All @@ -289,7 +292,8 @@ public void parsedHeader(HttpField field)
public boolean headerComplete()
{
HttpExchange exchange = getHttpExchange();
if (exchange == null)
unsolicited |= exchange == null;
if (unsolicited)
return false;

return !responseHeaders(exchange);
Expand All @@ -299,7 +303,8 @@ public boolean headerComplete()
public boolean content(ByteBuffer buffer)
{
HttpExchange exchange = getHttpExchange();
if (exchange == null)
unsolicited |= exchange == null;
if (unsolicited)
return false;

RetainableByteBuffer networkBuffer = this.networkBuffer;
Expand All @@ -321,7 +326,8 @@ public boolean contentComplete()
public void parsedTrailer(HttpField trailer)
{
HttpExchange exchange = getHttpExchange();
if (exchange == null)
unsolicited |= exchange == null;
if (unsolicited)
return;

exchange.getResponse().trailer(trailer);
Expand All @@ -331,8 +337,12 @@ public void parsedTrailer(HttpField trailer)
public boolean messageComplete()
{
HttpExchange exchange = getHttpExchange();
if (exchange == null)
if (exchange == null || unsolicited)
{
// We received an unsolicited response from the server.
getHttpConnection().close();
return false;
}

int status = exchange.getResponse().getStatus();
if (status != HttpStatus.CONTINUE_100)
Expand All @@ -349,7 +359,7 @@ public void earlyEOF()
{
HttpExchange exchange = getHttpExchange();
HttpConnectionOverHTTP connection = getHttpConnection();
if (exchange == null)
if (exchange == null || unsolicited)
connection.close();
else
failAndClose(new EOFException(String.valueOf(connection)));
Expand All @@ -359,7 +369,11 @@ public void earlyEOF()
public void badMessage(BadMessageException failure)
{
HttpExchange exchange = getHttpExchange();
if (exchange != null)
if (exchange == null || unsolicited)
{
getHttpConnection().close();
}
else
{
HttpResponse response = exchange.getResponse();
response.status(failure.getCode()).reason(failure.getReason());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,61 @@ public void onComplete(Result result)
assertArrayEquals(bytes, baos.toByteArray());
}

@ParameterizedTest
@ArgumentsSource(ScenarioProvider.class)
public void testUnsolicitedResponseBytesFromServer(Scenario scenario) throws Exception
{
String response = "" +
"HTTP/1.1 408 Request Timeout\r\n" +
"Content-Length: 0\r\n" +
"Connection: close\r\n" +
"\r\n";
testUnsolicitedBytesFromServer(scenario, response);
}

@ParameterizedTest
@ArgumentsSource(ScenarioProvider.class)
public void testUnsolicitedInvalidBytesFromServer(Scenario scenario) throws Exception
{
String response = "ABCDEF";
testUnsolicitedBytesFromServer(scenario, response);
}

private void testUnsolicitedBytesFromServer(Scenario scenario, String bytesFromServer) throws Exception
{
try (ServerSocket server = new ServerSocket(0))
{
HttpClientTransport transport = new HttpClientTransportOverHTTP(1);
transport.setConnectionPoolFactory(destination ->
{
ConnectionPool connectionPool = new DuplexConnectionPool(destination, 1, destination);
connectionPool.preCreateConnections(1);
return connectionPool;
});
startClient(scenario, transport, null);

String host = "localhost";
int port = server.getLocalPort();

// Resolve the destination which will pre-create a connection.
HttpDestination destination = client.resolveDestination(new Origin("http", host, port));

// Accept the connection and send an unsolicited 408.
try (Socket socket = server.accept())
{
OutputStream output = socket.getOutputStream();
output.write(bytesFromServer.getBytes(StandardCharsets.UTF_8));
output.flush();
}

// Give some time to the client to process the response.
Thread.sleep(1000);

AbstractConnectionPool pool = (AbstractConnectionPool)destination.getConnectionPool();
assertEquals(0, pool.getConnectionCount());
}
}

private void assertCopyRequest(Request original)
{
Request copy = client.copyRequest((HttpRequest)original, original.getURI());
Expand Down

0 comments on commit c88aba6

Please sign in to comment.