HTTP 400 and NPE in HttpParser for blank header value in Jetty 12.x

[mihalyr]

Jetty Version
12.0.5

Jetty Environment
ee10 but not relevant in this case

Java Version
21.0.2 (Corretto)

Question
After upgrading from 11.x to 12.x noticed a difference in header parsing. One of our tests for bad auth header values is now failing inside Jetty header parsing without reaching our handler. Below is a minimal reproducer with Jetty 12.0.5 and Apache HTTP client:

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.io.IOException;
import org.apache.http.HttpVersion;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.impl.client.HttpClients;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Response;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.handler.ErrorHandler;
import org.eclipse.jetty.util.Callback;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.junit.platform.commons.logging.Logger;
import org.junit.platform.commons.logging.LoggerFactory;

public class Jetty12HeaderTest {
  static final Logger log = LoggerFactory.getLogger(Jetty12HeaderTest.class);

  @ValueSource(strings = {"", " "})
  @ParameterizedTest
  void it_does_not_fail_on_blank_header_value(String headerValue) throws Exception {
    var server = new Server(0);
    server.setHandler(new Handler.Abstract() {
      @Override
      public boolean handle(final Request request, final Response response, final Callback callback) throws Exception {
        response.setStatus(200);
        callback.succeeded();
        return true;
      }
    });
    server.setErrorHandler(new ErrorHandler() {
      @Override
      protected void generateResponse(final Request request, final Response response, final int code,
          final String message, final Throwable cause, final Callback callback) throws IOException {
        log.error(cause, () -> "Error handler called with code " + code + " and message " + message);
        callback.succeeded();
      }
    });
    server.start();
    try (var client = HttpClients.createDefault()) {
      var request = new HttpPost(server.getURI().toString());
      request.setHeader("Authorization", headerValue);
      request.setProtocolVersion(HttpVersion.HTTP_1_0);
      try (var response = client.execute(request)) {
        assertEquals(200, response.getStatusLine().getStatusCode(),
            () -> "Response status was " + response.getStatusLine());
      }
    } finally {
      server.stop();
    }
  }
}

I expect Jetty to pass the blank " " header value just like the empty "" header value to the hander and let the handler decide what to do. This was the behavior in Jetty 11, but in Jetty 12 the blank value will throw a NPE inside Jetty header parser and result in HTTP 400 response.

Jan 18, 2024 1:48:34 P.M. Jetty12HeaderTest$2 generateResponse
SEVERE: Error handler called with code 400 and message Bad Request
org.eclipse.jetty.http.BadMessageException: 400: Bad Request
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1652)
	at org.eclipse.jetty.server.internal.HttpConnection.parseRequestBuffer(HttpConnection.java:580)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:404)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:971)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1201)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1156)
	at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.NullPointerException: Cannot invoke "String.length()" because "valueString" is null
	at org.eclipse.jetty.http.HttpParser$FieldCache.cacheable(HttpParser.java:2131)
	at org.eclipse.jetty.http.HttpParser.parsedHeader(HttpParser.java:1091)
	at org.eclipse.jetty.http.HttpParser.parseFields(HttpParser.java:1268)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1543)
	... 9 more

Could you please check if this is a bug or an expected behavior change in header parsing?

[gregw]

NPE is definitely a bug. Need to think about if we let the space through or not.

[mihalyr]

Here is the wire log for the success case:

wire - http-outgoing-0 >> "POST / HTTP/1.0[\r][\n]"
wire - http-outgoing-0 >> "Authorization: [\r][\n]"
wire - http-outgoing-0 >> "Content-Length: 0[\r][\n]"
wire - http-outgoing-0 >> "Host: 127.0.0.1:38945[\r][\n]"
wire - http-outgoing-0 >> "Connection: Keep-Alive[\r][\n]"
wire - http-outgoing-0 >> "User-Agent: Apache-HttpClient/4.5.13 (Java/21.0.2)[\r][\n]"
wire - http-outgoing-0 >> "Accept-Encoding: gzip,deflate[\r][\n]"
wire - http-outgoing-0 >> "[\r][\n]"
wire - http-outgoing-0 << "HTTP/1.1 200 OK[\r][\n]"
wire - http-outgoing-0 << "Server: Jetty(12.0.5)[\r][\n]"
wire - http-outgoing-0 << "Date: Thu, 18 Jan 2024 14:13:00 GMT[\r][\n]"
wire - http-outgoing-0 << "Connection: keep-alive[\r][\n]"
wire - http-outgoing-0 << "Content-Length: 0[\r][\n]"
wire - http-outgoing-0 << "[\r][\n]"

and the failure case:

wire - http-outgoing-1 >> "POST / HTTP/1.0[\r][\n]"
wire - http-outgoing-1 >> "Authorization:  [\r][\n]"
wire - http-outgoing-1 >> "Content-Length: 0[\r][\n]"
wire - http-outgoing-1 >> "Host: 127.0.0.1:37873[\r][\n]"
wire - http-outgoing-1 >> "Connection: Keep-Alive[\r][\n]"
wire - http-outgoing-1 >> "User-Agent: Apache-HttpClient/4.5.13 (Java/21.0.2)[\r][\n]"
wire - http-outgoing-1 >> "Accept-Encoding: gzip,deflate[\r][\n]"
wire - http-outgoing-1 >> "[\r][\n]"
wire - http-outgoing-1 << "HTTP/1.1 400 Bad Request[\r][\n]"
wire - http-outgoing-1 << "Server: Jetty(12.0.5)[\r][\n]"
wire - http-outgoing-1 << "Date: Thu, 18 Jan 2024 14:13:00 GMT[\r][\n]"
wire - http-outgoing-1 << "Cache-Control: must-revalidate,no-cache,no-store[\r][\n]"
wire - http-outgoing-1 << "Content-Length: 0[\r][\n]"
wire - http-outgoing-1 << "[\r][\n]"

[gregw]

Thanks. I've reproduced it. It is the double space in a cacheable field that we are not handling well. standby....

[gregw]

Fix merged and will be in the next release.