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

Improved handling of 100 Continue #12113

Merged
merged 7 commits into from
Aug 13, 2024
Merged

Improved handling of 100 Continue #12113

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
944b9e8
Improvements to HttpSender.
sbordet Jul 30, 2024
7964f40
Improved handling of 100 Continue.
sbordet Jul 31, 2024
c7674f6
Merged branch 'jetty-12.0.x' into 'fix/jetty-12.0.x/httpsender-proxy-…
sbordet Jul 31, 2024
4e9755d
Fixes after test failure.
sbordet Aug 1, 2024
6b307e1
Merged branch 'jetty-12.0.x' into 'fix/jetty-12.0.x/httpsender-proxy-…
sbordet Aug 2, 2024
e11ab97
Updates from review.
sbordet Aug 13, 2024
cd50711
Updates after review.
sbordet Aug 13, 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
10 changes: 7 additions & 3 deletions jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ContinueProtocolHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,14 @@ public String getName()
@Override
public boolean accept(Request request, Response response)
{
boolean is100 = response.getStatus() == HttpStatus.CONTINUE_100;
boolean expect100 = request.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString());
boolean handled100 = request.getAttributes().containsKey(ATTRIBUTE);
return (is100 || expect100) && !handled100;
if (handled100)
return false;
boolean is100 = response.getStatus() == HttpStatus.CONTINUE_100;
if (is100)
return true;
// Also handle non-100 responses, because we need to complete the request to complete the whole exchange.
return request.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString());
}

@Override
Expand Down
19 changes: 7 additions & 12 deletions jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import org.eclipse.jetty.client.Authentication;
import org.eclipse.jetty.client.AuthenticationStore;
import org.eclipse.jetty.client.BytesRequestContent;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.HttpProxy;
import org.eclipse.jetty.client.HttpRequestException;
Expand Down Expand Up @@ -146,7 +145,7 @@ protected void normalizeRequest(HttpRequest request)

// Make sure the path is there
String path = request.getPath();
if (path.trim().length() == 0)
if (path.trim().isEmpty())
sbordet marked this conversation as resolved.
Show resolved Hide resolved
{
path = "/";
request.path(path);
Expand Down Expand Up @@ -191,22 +190,15 @@ protected void normalizeRequest(HttpRequest request)

// Add content headers.
Request.Content content = request.getBody();
if (content == null)
{
request.body(new BytesRequestContent());
}
else
if (content != null)
{
if (!headers.contains(HttpHeader.CONTENT_TYPE))
{
String contentType = content.getContentType();
if (contentType == null)
contentType = getHttpClient().getDefaultRequestContentType();
if (contentType != null)
{
HttpField field = new HttpField(HttpHeader.CONTENT_TYPE, contentType);
request.addHeader(field);
}
request.addHeader(new HttpField(HttpHeader.CONTENT_TYPE, contentType));
}
long contentLength = content.getLength();
if (contentLength >= 0)
Expand All @@ -215,6 +207,9 @@ protected void normalizeRequest(HttpRequest request)
request.addHeader(new HttpField.LongValueHttpField(HttpHeader.CONTENT_LENGTH, contentLength));
}
}
// RFC 9110, section 10.1.1.
if (content == null || content.getLength() == 0)
request.headers(h -> h.remove(HttpHeader.EXPECT));
Copy link
Contributor

Choose a reason for hiding this comment

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

technically you should just remove the 100-continue value from the Expect header, but as there are currently no other defined values, this is probably OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. And we don't have an easy API in HttpFields to remove a single value.


// Cookies.
StringBuilder cookies = convertCookies(request.getCookies(), null);
Expand Down Expand Up @@ -243,7 +238,7 @@ private StringBuilder convertCookies(List<HttpCookie> cookies, StringBuilder bui
{
if (builder == null)
builder = new StringBuilder();
if (builder.length() > 0)
if (!builder.isEmpty())
builder.append("; ");
builder.append(cookie.getName()).append("=").append(cookie.getValue());
}
Expand Down
3 changes: 2 additions & 1 deletion jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ protected Action process() throws Throwable
action.run();

// Read the request content.
chunk = content.read();
chunk = content != null ? content.read() : Content.Chunk.EOF;
}
if (LOG.isDebugEnabled())
LOG.debug("Content {} for {}", chunk, request);
Expand All @@ -539,6 +539,7 @@ protected Action process() throws Throwable
{
// No content after the headers, demand.
demanded = true;
assert content != null;
content.demand(this::succeeded);
return Action.SCHEDULED;
}
Expand Down
9 changes: 5 additions & 4 deletions jetty-core/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,11 +393,12 @@ protected void addForwardedHeader(Request clientToProxyRequest, org.eclipse.jett

private boolean hasContent(Request clientToProxyRequest)
{
if (clientToProxyRequest.getLength() > 0)
long contentLength = clientToProxyRequest.getLength();
if (contentLength == 0)
return false;
if (contentLength > 0)
return true;
HttpFields headers = clientToProxyRequest.getHeaders();
return headers.get(HttpHeader.CONTENT_TYPE) != null ||
headers.get(HttpHeader.TRANSFER_ENCODING) != null;
return clientToProxyRequest.getHeaders().get(HttpHeader.TRANSFER_ENCODING) != null;
}

private boolean expects100Continue(Request clientToProxyRequest)
Expand Down
8 changes: 4 additions & 4 deletions ...y-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,8 +1117,6 @@ public String toString()
*/
public static class ChannelResponse implements Response, Callback
{
private static final CompletableFuture<Void> UNEXPECTED_100_CONTINUE = CompletableFuture.failedFuture(new IllegalStateException("100 not expected"));
private static final CompletableFuture<Void> COMMITTED_100_CONTINUE = CompletableFuture.failedFuture(new IllegalStateException("Committed"));
private final ChannelRequest _request;
private final ResponseHttpFields _httpFields;
protected int _status;
Expand Down Expand Up @@ -1408,12 +1406,14 @@ public CompletableFuture<Void> writeInterim(int status, HttpFields headers)
if (status == HttpStatus.CONTINUE_100)
{
if (!httpChannelState._expects100Continue)
return UNEXPECTED_100_CONTINUE;
return CompletableFuture.failedFuture(new IllegalStateException("100 not expected"));
if (_request.getLength() == 0)
gregw marked this conversation as resolved.
Show resolved Hide resolved
return CompletableFuture.completedFuture(null);
httpChannelState._expects100Continue = false;
}

if (_httpFields.isCommitted())
return status == HttpStatus.CONTINUE_100 ? COMMITTED_100_CONTINUE : CompletableFuture.failedFuture(new IllegalStateException("Committed"));
return CompletableFuture.failedFuture(new IllegalStateException("Committed"));
if (_writeCallback != null)
return CompletableFuture.failedFuture(new WritePendingException());

Expand Down
9 changes: 6 additions & 3 deletions ...e10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,12 @@ protected void onProxyRewriteFailed(HttpServletRequest clientRequest, HttpServle

protected boolean hasContent(HttpServletRequest clientRequest)
{
return clientRequest.getContentLength() > 0 ||
clientRequest.getContentType() != null ||
clientRequest.getHeader(HttpHeader.TRANSFER_ENCODING.asString()) != null;
long contentLength = clientRequest.getContentLengthLong();
if (contentLength == 0)
return false;
if (contentLength > 0)
return true;
return clientRequest.getHeader(HttpHeader.TRANSFER_ENCODING.asString()) != null;
}

protected boolean expects100Continue(HttpServletRequest request)
Expand Down
79 changes: 79 additions & 0 deletions jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java
View file
Open in desktop
sbordet marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
import java.io.PrintWriter;
import java.io.Writer;
import java.net.ConnectException;
import java.net.InetSocketAddress;
import java.nio.ByteBuffer;
import java.nio.channels.SocketChannel;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
Expand Down Expand Up @@ -80,6 +82,7 @@
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.HttpScheme;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.HttpConnectionFactory;
Expand Down Expand Up @@ -1703,4 +1706,80 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
assertEquals(HttpStatus.OK_200, response.getStatus());
}
}

@ParameterizedTest
@MethodSource("impls")
public void testExpect100ContinueContentLengthZero(Class<? extends ProxyServlet> proxyServletClass) throws Exception
{
testExpect100ContinueNoRequestContent(proxyServletClass, false);
}

@ParameterizedTest
@MethodSource("impls")
public void testExpect100ContinueEmptyChunkedContent(Class<? extends ProxyServlet> proxyServletClass) throws Exception
{
testExpect100ContinueNoRequestContent(proxyServletClass, true);
}

private void testExpect100ContinueNoRequestContent(Class<? extends ProxyServlet> proxyServletClass, boolean chunked) throws Exception
{
startServer(new HttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
{
// Send the 100 Continue.
ServletInputStream input = request.getInputStream();
// Echo the content.
IO.copy(input, response.getOutputStream());
}
});
startProxy(proxyServletClass);

String authority = "localhost:" + serverConnector.getLocalPort();
for (int i = 0; i < 50; i++)
{
try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", proxyConnector.getLocalPort())))
{
String request;
if (chunked)
{
request = """
POST http://$A/ HTTP/1.1
Host: $A
Expect: 100-Continue
Transfer-Encoding: chunked

0

""";
}
else
{
request = """
POST http://$A/ HTTP/1.1
Host: $A
Expect: 100-Continue
Content-Length: 0

""";
}
request = request.replace("$A", authority);
client.write(StandardCharsets.UTF_8.encode(request));

HttpTester.Input input = HttpTester.from(client);
HttpTester.Response response1 = HttpTester.parseResponse(input);
if (chunked)
{
assertEquals(HttpStatus.CONTINUE_100, response1.getStatus());
HttpTester.Response response2 = HttpTester.parseResponse(input);
assertEquals(HttpStatus.OK_200, response2.getStatus());
}
else
{
assertEquals(HttpStatus.OK_200, response1.getStatus());
}
}
}
}
}
66 changes: 63 additions & 3 deletions ...ts/src/test/java/org/eclipse/jetty/ee10/test/client/transport/HttpClientContinueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.net.ServerSocket;
import java.net.Socket;
import java.nio.ByteBuffer;
import java.nio.channels.SocketChannel;
import java.nio.charset.StandardCharsets;
import java.util.EnumSet;
import java.util.Random;
Expand All @@ -45,11 +46,13 @@
import org.eclipse.jetty.client.Request;
import org.eclipse.jetty.client.Response;
import org.eclipse.jetty.client.Result;
import org.eclipse.jetty.client.StringRequestContent;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpHeaderValue;
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.server.NetworkConnector;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.IO;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -163,7 +166,6 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
.body(content)
.timeout(5, TimeUnit.SECONDS)
.send();

}

assertNotNull(response);
Expand Down Expand Up @@ -233,14 +235,14 @@ public long getLength()
@MethodSource("transportsNoFCGI")
public void testExpect100ContinueWithContentRespond417ExpectationFailed(Transport transport) throws Exception
{
testExpect100ContinueWithContentRespondError(transport, 417);
testExpect100ContinueWithContentRespondError(transport, HttpStatus.EXPECTATION_FAILED_417);
}

@ParameterizedTest
@MethodSource("transportsNoFCGI")
public void testExpect100ContinueWithContentRespond413RequestEntityTooLarge(Transport transport) throws Exception
{
testExpect100ContinueWithContentRespondError(transport, 413);
testExpect100ContinueWithContentRespondError(transport, HttpStatus.PAYLOAD_TOO_LARGE_413);
}

private void testExpect100ContinueWithContentRespondError(Transport transport, int error) throws Exception
Expand Down Expand Up @@ -796,6 +798,64 @@ protected void service(HttpServletRequest request, HttpServletResponse response)
assertTrue(clientLatch.await(5, TimeUnit.SECONDS));
}

@ParameterizedTest
@MethodSource("transportsNoFCGI")
public void testExpect100ContinueWithContentLengthZeroExpectIsRemoved(Transport transport) throws Exception
{
start(transport, new HttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response)
{
assertEquals(0, request.getContentLengthLong());
// The Expect header must have been removed by the client.
assertNull(request.getHeader(HttpHeader.EXPECT.asString()));
}
});

ContentResponse response = client.newRequest(newURI(transport))
.headers(headers -> headers.put(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()))
.body(new StringRequestContent(""))
.timeout(5, TimeUnit.SECONDS)
.send();

assertEquals(HttpStatus.OK_200, response.getStatus());
}

@Test
public void testExpect100ContinueWithContentLengthZero() throws Exception
{
startServer(Transport.HTTP, new HttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
assertEquals(0, request.getContentLengthLong());
assertNotNull(request.getHeader(HttpHeader.EXPECT.asString()));

// Trigger the 100-Continue logic.
// The 100 continue will not be sent, since there is no request content.
ServletInputStream input = request.getInputStream();
assertEquals(-1, input.read());
}
});

try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", ((NetworkConnector)connector).getLocalPort())))
{
String request = """
GET / HTTP/1.1
Host: localhost
Expect: 100-Continue
Content-Length: 0

""";
client.write(StandardCharsets.UTF_8.encode(request));

HttpTester.Response response = HttpTester.parseResponse(HttpTester.from(client));
assertEquals(HttpStatus.OK_200, response.getStatus());
}
}

@Test
public void testExpect100ContinueWithTwoResponsesInOneRead() throws Exception
{
Expand Down
9 changes: 6 additions & 3 deletions ...y-ee9/jetty-ee9-proxy/src/main/java/org/eclipse/jetty/ee9/proxy/AbstractProxyServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,9 +459,12 @@ protected void onProxyRewriteFailed(HttpServletRequest clientRequest, HttpServle

protected boolean hasContent(HttpServletRequest clientRequest)
{
return clientRequest.getContentLength() > 0 ||
clientRequest.getContentType() != null ||
clientRequest.getHeader(HttpHeader.TRANSFER_ENCODING.asString()) != null;
long contentLength = clientRequest.getContentLengthLong();
if (contentLength == 0)
return false;
if (contentLength > 0)
return true;
return clientRequest.getHeader(HttpHeader.TRANSFER_ENCODING.asString()) != null;
}

protected boolean expects100Continue(HttpServletRequest request)
Expand Down
Loading