Ambiguous segment in URI in DELETE /a/projects/foo/branches/refs%2Fheads%2Ftest request after upgrade from 10.0.0 to 10.0.2 #6132
Comments
|
@lorban can you look at this one. |
|
After looking at the logs, it seems that Here's the original cause, which seems to be related to our recent ambiguous URL fix: |
|
The 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 |
|
@davido you may want to try calling |
|
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 the branch named: "refs/heads/test". URL encoded is spelled "refs%2Fheads%2Ftest", obviously. This seems to be related to: 06e1a7e . |
davido
changed the title
|
I think that setting a configuration that allows |
|
Specifically, I recommend using @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: 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.... |
|
Thanks for the fix! Just to confirm that after upgrading to 10.0.3 and removing the workaround: |
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:
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
.projectand.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
The text was updated successfully, but these errors were encountered: