[arturobernalg]

A message with both headers may indicate request smuggling or response splitting and "ought to be handled as an error" (RFC 9112 section 6.1). DefaultContentLengthStrategy let Transfer-Encoding win, and the message kept both headers.

Against real servers on master, a request with Content-Length: 6, Transfer-Encoding: chunked and a pipelined GET: the classic server answered 200 and the handler saw both headers; the async server served both requests. A classic client accepted a response carrying both headers.

determineLength now throws ProtocolException when both are present: the server answers 400 with Connection: close, the handler is not called, and the client fails the response.

The async server did not answer a message whose framing could not be determined, it dropped the connection (for instance for Transfer-Encoding: gzip). A small package-private hook in the HTTP/1 duplexer lets ServerHttp1StreamDuplexer terminate the exchange as it does for parse errors, so it now responds as the classic server does, 400 here and 501 for an unsupported coding. The "before" figures for the gzip case come from a raw-socket reproducer, the new tests were not run against the unmodified tree.

Tests: unit tests for the strategy and raw-socket integration tests for the classic and the async server, over plain HTTP and TLS.

Compatibility: both headers together are a protocol violation that well-known clients and servers do not emit, but I have not surveyed real ones. The check also applies to responses and to messages written by a handler or proxy that sets both headers, which would now get a ProtocolException; such a proxy would need to drop Content-Length when forwarding.

[ok2c]

@arturobernalg The request smuggling should be prevented by ConnectionReuseStrategy. The default impl treats connections after a response with both headers present as non-reusable. I am not favor of this change.

Please change your priorities.

[arturobernalg]

Thanks, Oleg. Understood, I am closing this one. For the record, on master the async server answers a pipelined GET that follows a request with both headers (handler called twice), while the classic server closes after the first response. I leave it at that.

[ok2c]

For the record, on master the async server answers a pipelined GET that follows a request with both headers (handler called twice), while the classic server closes after the first response.

@arturobernalg Please submit a reproducer once you get a chance.

[arturobernalg]

Reproducer, run against master 450ef92. It sends a POST carrying both Content-Length and Transfer-Encoding, followed by a pipelined GET, to the classic and to the async server.

import java.io.InputStream;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketTimeoutException;
import java.nio.charset.StandardCharsets;
import java.util.concurrent.atomic.AtomicInteger;

import org.apache.hc.core5.http.EntityDetails;
import org.apache.hc.core5.http.HttpRequest;
import org.apache.hc.core5.http.Message;
import org.apache.hc.core5.http.URIScheme;
import org.apache.hc.core5.http.impl.bootstrap.AsyncServerBootstrap;
import org.apache.hc.core5.http.impl.bootstrap.HttpAsyncServer;
import org.apache.hc.core5.http.impl.bootstrap.HttpServer;
import org.apache.hc.core5.http.impl.bootstrap.ServerBootstrap;
import org.apache.hc.core5.http.io.entity.StringEntity;
import org.apache.hc.core5.http.nio.AsyncRequestConsumer;
import org.apache.hc.core5.http.nio.AsyncServerRequestHandler;
import org.apache.hc.core5.http.nio.entity.StringAsyncEntityConsumer;
import org.apache.hc.core5.http.nio.support.BasicRequestConsumer;
import org.apache.hc.core5.http.nio.support.BasicResponseProducer;
import org.apache.hc.core5.http.nio.support.BasicServerExchangeHandler;
import org.apache.hc.core5.http.protocol.HttpContext;
import org.apache.hc.core5.io.CloseMode;

public class PipelinedTeCl {

    static final String RAW = "POST /a HTTP/1.1\r\nHost: localhost\r\nContent-Length: 6\r\n"
            + "Transfer-Encoding: chunked\r\n\r\n5\r\nAAAAA\r\n0\r\n\r\n"
            + "GET /b HTTP/1.1\r\nHost: localhost\r\n\r\n";

    static int responses(final int port) throws Exception {
        try (Socket s = new Socket("localhost", port)) {
            s.setSoTimeout(2000);
            s.getOutputStream().write(RAW.getBytes(StandardCharsets.ISO_8859_1));
            final StringBuilder sb = new StringBuilder();
            final InputStream in = s.getInputStream();
            final byte[] b = new byte[1024];
            try {
                int n;
                while ((n = in.read(b)) != -1) {
                    sb.append(new String(b, 0, n, StandardCharsets.ISO_8859_1));
                }
            } catch (final SocketTimeoutException ignore) {
            }
            return sb.toString().split("HTTP/1.1 200 OK", -1).length - 1;
        }
    }

    public static void main(final String[] args) throws Exception {
        final AtomicInteger calls = new AtomicInteger();

        final HttpServer classic = ServerBootstrap.bootstrap()
                .setCanonicalHostName("localhost")
                .register("*", (request, response, context) -> {
                    calls.incrementAndGet();
                    response.setEntity(new StringEntity("ok"));
                })
                .create();
        classic.start();
        System.out.println("classic server: responses=" + responses(classic.getLocalPort())
                + " handler calls=" + calls.get());
        classic.close(CloseMode.IMMEDIATE);

        calls.set(0);
        final HttpAsyncServer async = AsyncServerBootstrap.bootstrap()
                .setCanonicalHostName("localhost")
                .register("*", () -> new BasicServerExchangeHandler<>(
                        new AsyncServerRequestHandler<Message<HttpRequest, String>>() {

                            @Override
                            public AsyncRequestConsumer<Message<HttpRequest, String>> prepare(
                                    final HttpRequest request, final EntityDetails entityDetails,
                                    final HttpContext context) {
                                return new BasicRequestConsumer<>(new StringAsyncEntityConsumer());
                            }

                            @Override
                            public void handle(
                                    final Message<HttpRequest, String> message, final ResponseTrigger trigger,
                                    final HttpContext context) throws java.io.IOException,
                                    org.apache.hc.core5.http.HttpException {
                                calls.incrementAndGet();
                                trigger.submitResponse(new BasicResponseProducer(200, "ok"), context);
                            }
                        }))
                .create();
        async.start();
        final int port = ((InetSocketAddress) async.listen(new InetSocketAddress(0), URIScheme.HTTP)
                .get().getAddress()).getPort();
        System.out.println("async server:   responses=" + responses(port) + " handler calls=" + calls.get());
        async.close(CloseMode.IMMEDIATE);
    }
}

Output:

classic server: responses=1 handler calls=1
async server:   responses=2 handler calls=2

Both servers frame the request by Transfer-Encoding, so answering the pipelined GET is correct under that framing. The difference is that the classic server closes the connection after the first response, as the reuse strategy says, while the async server has already read the next request by then and answers it too. It only matters behind a front end that frames by Content-Length. I do not claim more than that.

[ok2c]

@arturobernalg This is most likely due to both messages transmitted as a single TCP frame. While plausible I would consider this highly unlikely scenario in real life and even then still within the tolerance of the spec.

[arturobernalg]

Thanks, Oleg. I also ran it behind Apache httpd 2.4.68 with mod_proxy: it drops the Transfer-Encoding, forwards Content-Length and does not pass the second request on, so I have no real example of smuggling. Agreed that it is within the tolerance of the spec.