Ambiguous segment in URI in DELETE /a/projects/foo/branches/refs%2Fheads%2Ftest request after upgrade from 10.0.0 to 10.0.2

[davido]

Jetty version

10.0.0 vs. 10.0.02

Java version

Open JDK 11

OS type/version

Linux

Description

After upgrading Gerrit Code Review from 10.0.0 to 10.0.2: [1] DELETE requests are failing with 400.

If I activate debug logging in Jetty, I can see this ISE on Jetty 10.0.2: [2]. All is fine in Jetty 10.0.0 on Gerrit@HEAD:

DEBUG org.eclipse.jetty.server.HttpConnection : abort HttpConnection@32d5a0c9::SocketChannelEndPoint@22741abc{l=/127.0.0.1:35311,r=/127.0.0.1:37224,OSHUT,fill=-,flush=-,to=14/30000}{io=0/0,kio=0,kro=1}->HttpConnection@32d5a0c9[p=HttpParser{s=CLOSE,0 of -1},g=HttpGenerator@6d8711da{s=END}]=>HttpChannelOverHttp@b78266e{s=HttpChannelState@7c286cc6{s=HANDLING rs=BLOCKING os=ABORTED is=IDLE awp=false se=false i=true al=0},r=2,c=false/false,a=HANDLING,uri=null,age=35} {}
java.lang.IllegalStateException: s=HANDLING rs=BLOCKING os=COMPLETED is=IDLE awp=false se=false i=true al=0
	at org.eclipse.jetty.server.HttpChannelState.recycle(HttpChannelState.java:1025)
	at org.eclipse.jetty.server.Request.recycle(Request.java:1799)
	at org.eclipse.jetty.server.HttpChannel.recycle(HttpChannel.java:339)
	at org.eclipse.jetty.server.HttpChannelOverHttp.recycle(HttpChannelOverHttp.java:557)
	at org.eclipse.jetty.server.HttpConnection.onCompleted(HttpConnection.java:452)
	at org.eclipse.jetty.server.HttpChannel.onCompleted(HttpChannel.java:865)
	at org.eclipse.jetty.server.HttpChannel.onBadMessage(HttpChannel.java:913)
	at org.eclipse.jetty.server.HttpChannelOverHttp.badMessage(HttpChannelOverHttp.java:182)
	at org.eclipse.jetty.http.HttpParser.badMessage(HttpParser.java:1636)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1618)
	at org.eclipse.jetty.server.HttpConnection.parseRequestBuffer(HttpConnection.java:379)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:272)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:319)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
	at org.eclipse.jetty.io.SocketChannelEndPoint$1.run(SocketChannelEndPoint.java:101)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:894)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1038)
	at java.base/java.lang.Thread.run(Thread.java:834)

To reproduce, clone gerrit recursively, apply this CL: [1] and run from the command line:

$ bazel test //javatests/com/google/gerrit/acceptance/rest/project:DeleteBranchIT

or, if you prefer to debug in Eclipse, generate .project and .classpath, running:

$ tools/eclipse/project.py

And import gerrit workspace in Eclipse IDE. Then open DeleteBranchIT.java, and Run "debug as JUint test".

[1] https://gerrit-review.googlesource.com/c/gerrit/+/238383
[2] jetty_logging.zip

[gregw]

@lorban can you look at this one.

[lorban]

After looking at the logs, it seems that IllegalStateException is a red herring caused by a prior exception (although we may want to improve that code to avoid generating that IllegalStateException).

Here's the original cause, which seems to be related to our recent ambiguous URL fix:

DEBUG org.eclipse.jetty.http.HttpParser : Parse exception: HttpParser{s=CONTENT,0 of -1} for HttpChannelOverHttp@b78266e{s=HttpChannelState@7c286cc6{s=IDLE rs=BLOCKING os=OPEN is=IDLE awp=false se=false i=true al=0},r=1,c=false/false,a=IDLE,uri=null,age=15}
org.eclipse.jetty.http.BadMessageException: 400: Ambiguous segment in URI
	at org.eclipse.jetty.server.Request.setMetaData(Request.java:1702)
	at org.eclipse.jetty.server.HttpChannel.onRequest(HttpChannel.java:794)
	at org.eclipse.jetty.server.HttpChannelOverHttp.headerComplete(HttpChannelOverHttp.java:332)
	at org.eclipse.jetty.http.HttpParser.parseFields(HttpParser.java:1226)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1511)
	at org.eclipse.jetty.server.HttpConnection.parseRequestBuffer(HttpConnection.java:379)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:272)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:319)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
	at org.eclipse.jetty.io.SocketChannelEndPoint$1.run(SocketChannelEndPoint.java:101)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:894)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1038)
	at java.base/java.lang.Thread.run(Thread.java:834)

[joakime]

The gerrit_jetty_10.0.0_debug_session_ok.txt shows the DELETE requests as ...

DELETE /a/projects/NURaYiok/branches/refs%2Fheads%2Ftest HTTP/1.1
Host: localhost:46117
Connection: keep-alive
User-Agent: Apache-HttpClient/4.5.2 (Java/11.0.10)
Accept-Encoding: gzip,deflate

[lorban]

@davido you may want to try calling setUriCompliance(UriCompliance.RFC3986) on your HttpConfiguration to allow ambiguous segments but you may want to check GHSA-v7ff-8wcx-gmc5 first for the implications of such setting.

[davido]

@lorban

Thanks. This diff fixed it:

diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
index 89b4228c3f..3c641e2059 100644
--- a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
+++ b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
@@ -49,6 +49,8 @@ import javax.servlet.Filter;
 import javax.servlet.http.HttpSessionEvent;
 import javax.servlet.http.HttpSessionListener;
 import org.eclipse.jetty.http.HttpScheme;
+import org.eclipse.jetty.http.HttpURI;
+import org.eclipse.jetty.http.UriCompliance;
 import org.eclipse.jetty.io.ConnectionStatistics;
 import org.eclipse.jetty.jmx.MBeanContainer;
 import org.eclipse.jetty.server.Connector;
@@ -362,7 +364,7 @@ public class JettyServer {
         config.addCustomizer(new ForwardedRequestCustomizer());
         config.addCustomizer(
             (connector, channelConfig, request) -> {
-              request.setScheme(HttpScheme.HTTPS.asString());
+              request.setHttpURI(HttpURI.build(request.getHttpURI()).scheme(HttpScheme.HTTPS));
               request.setSecure(true);
             });
         c = newServerConnector(server, acceptors, config);
@@ -417,6 +419,7 @@ public class JettyServer {
     config.setRequestHeaderSize(requestHeaderSize);
     config.setSendServerVersion(false);
     config.setSendDateHeader(true);
+    config.setUriCompliance(UriCompliance.RFC3986);
     return config;
   }

But is that really has to be that hard?

This request is saying:

  DELETE /a/projects/NURaYiok/branches/refs%2Fheads%2Ftest

delete the branch named: "refs/heads/test". URL encoded is spelled "refs%2Fheads%2Ftest", obviously.

This seems to be related to: 06e1a7e .

[gregw]

I think that setting a configuration that allows %2f, but not ..; or %2e or %2e would be safer than pure RFC3986 support

[gregw]

Specifically, I recommend using UriCompliance.from("0,AMBIGUOUS_PATH_SEPARATOR")

@joakime, @sbordet, @lorban : given that this is proving more common than we thought, should we revert to the above compliance being the default?

That would mean that the only segments that by default get hit with a direct 400 would be: ..; which is a silly segment that can only really be intended as a hack; %2e and %2e%2e which are rare as browser take them out anyway.

Also, we should have a better way to build compliances without using strings (which are not checked by the compiler).... let me prepare a PR for all of the above....