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.

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

Jetty 12: 400: Ambiguous URI path encoding for path <%=FOO%>~1 (encoded: %3C%25%3DFOO%25%3E%7E1) #11890

Closed
davido opened this issue Jun 8, 2024 · 11 comments
Labels
Bug For general bugs on Jetty side

Comments

@davido
Copy link

davido commented Jun 8, 2024

Jetty version(s)
Jetty 12.0.10

Jetty Environment
ee10

Java version/vendor (use: java -version)
JDK21

OS type/version
Linux

Description

We are migrating Gerrit Code Review project to Jetty 12.

One test is failing with encoding problem.
This is the related Gerrit change.
See also this SO question.

gerrit/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java

  @Test
  public void invalidProjectChangeNumberReturnsNotFound() throws Exception {
    IdString fromDecoded = IdString.fromDecoded("<%=FOO%>~1");
    RestResponse res =
        adminRestSession.get(changeDetail(fromDecoded.encoded()));
    res.assertNotFound();
  }

where fromDecoded: %3C%25%3DFOO%25%3E%7E1.
And the whole endpoint is /changes/%3C%25%3DFOO%25%3E%7E1/detail.

Starting from Jetty 12 this path is rejected with this exception:

org.eclipse.jetty.http.BadMessageException: 400: Ambiguous URI path encoding
	at org.eclipse.jetty.server.internal.HttpConnection$HttpStreamOverHTTP1.headerComplete(HttpConnection.java:1189)
	at org.eclipse.jetty.server.internal.HttpConnection$RequestHandler.headerComplete(HttpConnection.java:954)
	at org.eclipse.jetty.http.HttpParser.parseFields(HttpParser.java:1345)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1641)
	at org.eclipse.jetty.server.internal.HttpConnection.parseRequestBuffer(HttpConnection.java:560)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:383)
	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:979)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164)
	at java.base/java.lang.Thread.run(Thread.java:1583)

So that the test is failing with HTTP status mismatch:

Time: 63.949
There was 1 failure:
1) invalidProjectChangeNumberReturnsNotFound(com.google.gerrit.acceptance.rest.change.ChangeIdIT)
Expected status code 404
value of: getStatusCode()
expected: 404
but was : 400
	at com.google.gerrit.acceptance.RestResponse.assertStatus(RestResponse.java:59)
	at com.google.gerrit.acceptance.RestResponse.assertNotFound(RestResponse.java:67)
	at com.google.gerrit.acceptance.rest.change.ChangeIdIT.invalidProjectChangeNumberReturnsNotFound(ChangeIdIT.java:54)

FAILURES!!!
Tests run: 517,  Failures: 1

There are even more failures with the same encoding pattern, so that I'm posting it here, instead of filing new issue:

Also for this project name, where trailing slash was encoded:
com.google.gerrit.acceptance.rest.project.CreateChangeIT_matchingProjectWithTrailingSlashIsAccepted_project%2F
is failing with the same exception:

Also deleting of this git branch is failing now in REST endpoint, where / was encoded:

/projects/hxkePMGw/branches/refs%2Fheads%2Ftest.

How to reproduce?

Clone gerrit code review project, apply this pending CL

Fetch patch set 11 of this change:

  $> git fetch https://gerrit.googlesource.com/gerrit refs/changes/80/424580/11 && git checkout FETCH_HEAD

and run:

  $> bazel test //javatests/com/google/gerrit/acceptance/rest/change:rest_change_other \
       //javatests/com/google/gerrit/acceptance/rest/project:CreateChangeIT \
       //javatests/com/google/gerrit/acceptance/rest/project:DeleteBranchIT
@davido davido added the Bug For general bugs on Jetty side label Jun 8, 2024
@paulrutter
Copy link

See #11448 (comment) for a workaround, although proceed with caution as it's not spec compliant.

@davido
Copy link
Author

davido commented Jun 9, 2024

Thanks for the pointer. If we cannot pass '/' as part of the URI over HTTP any more, we can cancel Gerrit Code Review project.

DELETE '/projects/gmguilbA/branches/refs%2Fheads%2Ftest'

This is a usual request to delete git branch with the name: refs/heads/test.

I tried to set httpConfig.setUriCompliance(UriCompliance.from(UriCompliance.NO_VIOLATION)).
And I also tried to set: servletHandler.setDecodeAmbiguousURIs(true).

It doesn't seem to be respected.

For one I see, that the violation Violation.AMBIGUOUS_PATH_SEPARATOR is unconditionally added in HttpURI:

./jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java

    switch (encodedValue)
    {
        case 0:
            // Byte 0 cannot be present in a UTF-8 sequence in any position
            // other than as the NUL ASCII byte which we do not wish to allow.
            throw new IllegalArgumentException("Illegal character in path");
        case '/':
            addViolation(Violation.AMBIGUOUS_PATH_SEPARATOR);
            break;
        case '%':
            addViolation(Violation.AMBIGUOUS_PATH_ENCODING);
            break;
        default:
            if (encodedValue < __suspiciousPathCharacters.length && __suspiciousPathCharacters[encodedValue])
                addViolation(Violation.SUSPICIOUS_PATH_CHARACTERS);
            break;
    }

with this stack trace:

Thread [HTTP-44] (Suspended (breakpoint at line 1574 in HttpURI$Mutable))	
	HttpURI$Mutable.addViolation(UriCompliance$Violation) line: 1574	
	HttpURI$Mutable.parse(HttpURI$Mutable$State, String) line: 1339	
	HttpURI$Mutable.pathQuery(String) line: 999	
	HttpURI.build(String, String) line: 120	
	HttpConnection$HttpStreamOverHTTP1.<init>(HttpConnection, String, String, HttpVersion) line: 1103	
	HttpConnection.newHttpStream(String, String, HttpVersion) line: 191	
	HttpConnection$RequestHandler.startRequest(String, String, HttpVersion) line: 938	
	HttpParser.parseLine(ByteBuffer) line: 917	
	HttpParser.parseNext(ByteBuffer) line: 1634	
	HttpConnection.parseRequestBuffer() line: 560	
	HttpConnection.onFillable() line: 383	
	AbstractConnection$ReadCallback.succeeded() line: 322	
	AbstractEndPoint$1(FillInterest).fillable() line: 99	
	SelectableChannelEndPoint$1.run() line: 53	
	QueuedThreadPool.runJob(Runnable) line: 979	
	QueuedThreadPool$Runner.doRunJob(Runnable) line: 1209	
	QueuedThreadPool$Runner.run() line: 1164	
	Thread.runWith(Object, Runnable) line: 1596	
	Thread.run() line: 1583	

For another, the bad request exception (Status 400) is unconditionally thrown in HttpConnection.java:1189:

./jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

    String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, getHttpChannel().getComplianceViolationListener());
     if (badMessage != null)
         throw new BadMessageException(badMessage);

with this stack trace:

org.eclipse.jetty.http.BadMessageException: 400: Ambiguous URI path separator
	at org.eclipse.jetty.server.internal.HttpConnection$HttpStreamOverHTTP1.headerComplete(HttpConnection.java:1189)
	at org.eclipse.jetty.server.internal.HttpConnection$RequestHandler.headerComplete(HttpConnection.java:954)
	at org.eclipse.jetty.http.HttpParser.parseFields(HttpParser.java:1345)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1641)
	at org.eclipse.jetty.server.internal.HttpConnection.parseRequestBuffer(HttpConnection.java:560)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:383)
	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.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:478)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:441)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:293)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.run(AdaptiveExecutionStrategy.java:201)
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:311)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164)
	at java.base/java.lang.Thread.run(Thread.java:1583)

What am I missing?

In case it helps for analysis, here is the complete debug log.

@joakime
Copy link
Contributor

joakime commented Jun 9, 2024

Thanks for the pointer. If we cannot pass '/' as part of the URI over HTTP any more, we can cancel Gerrit Code Review project.

You cannot pass a ambiguous / (encoded as either %2F or %252F) as part of the path section of the URI over HTTP any more with Servlet 6 (or anything in ee10+, including REST).

But you can pass that encoded reference as part of the query section of a URI.

@joakime
Copy link
Contributor

joakime commented Jun 9, 2024

I tried to set httpConfig.setUriCompliance(UriCompliance.from(UriCompliance.NO_VIOLATION)). And I also tried to set: servletHandler.setDecodeAmbiguousURIs(true).

UriCompliance.NO_VIOLATION is a mode that will not allow any violation listed in UriCompliance.Violation.

Use this instead.

httpConfig.setUriCompliance(UriCompliance.from(Set.of(
    UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR,
    UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING)));

That says you are allowing AMBIGUOUS_PATH_SEPARATOR and AMBIGUOUS_PATH_ENCODING through.
This merely allows the bad behavior through the HttpParser (and Request Dispatcher, and Location lines, and Redirects, and Security Constraints, etc... Pretty much anywhere that needs the original URI to formulate the new URI or where a new path is provided into HTTP to handle things in HTTP)

This answer is pretty much from #11448 (comment) and also linked example code.

For one I see, that the violation Violation.AMBIGUOUS_PATH_SEPARATOR is unconditionally added in HttpURI:

That's where the violation is detected, and noted, it being enforced is elsewhere in the code.

For another, the bad request exception (Status 400) is unconditionally thrown in HttpConnection.java:1189:

That's one of the enforcement locations (not the only one), and will honor configuration present in HttpConfiguration.getUriCompliance() to determine if the enforcement action is applied.

Thanks for the pointer. If we cannot pass '/' as part of the URI over HTTP any more, we can cancel Gerrit Code Review project.

DELETE '/projects/gmguilbA/branches/refs%2Fheads%2Ftest'

Note that you cannot pass AMBIGUOUS path segments, meaning the %2F or using %252F in the path section of a URI (there's no such restriction in the query section).
If that path was just /projects/gmguilbA/branches/refs/heads/test then it would be 100% valid.

In Servlet terms, a url-pattern of /projects/* would accept that path happily.
The "pathInfo" for the request path /projects/gmguilbA/branches/refs/heads/test on url-pattern of /projects/* would return /gmguilbA/branches/refs/heads/test and you would need to break down/parse that path further to know what the branch reference is.

@davido
Copy link
Author

davido commented Jun 10, 2024

UriCompliance.NO_VIOLATION is a mode that will not allow any violation listed in UriCompliance.Violation.

Use this instead.

httpConfig.setUriCompliance(UriCompliance.from(Set.of(
UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR,
UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING)));

Thanks, I misread the javadoc. It works now as expected.

DELETE '/projects/gmguilbA/branches/refs%2Fheads%2Ftest'

Note that you cannot pass AMBIGUOUS path segments, meaning the %2F or using %252F in the path section of a URI (there's no such restriction in the query section).
If that path was just /projects/gmguilbA/branches/refs/heads/test then it would be 100% valid.

But then the API would be ambiguous:

  DELETE /projects/foo/bar/baz/branches/refs/heads/test

Would this request delete project (aka git repository) with the name: foo/bar/baz/branches/refs/heads/test, or would this request delete branch: refs/heads/test in project: foo/bar/baz?

@joakime
Copy link
Contributor

joakime commented Jun 10, 2024

But then the API would be ambiguous:

  DELETE /projects/foo/bar/baz/branches/refs/heads/test

Would this request delete project (aka git repository) with the name: foo/bar/baz/branches/refs/heads/test, or would this request delete branch: refs/heads/test in project: foo/bar/baz?

This is the nature of HTTP, and the URI specs.
The requested resource path is just a path.
The minute you start playing games with the path with using ambiguous things like %2F or %252F you are in a world of hurt.

The URI spec in your case should have used a forbidden character (to git) as a separator, not a /.

Example: both : and ~ are allowed "in the raw" for a URI/URL. But both characters are not allowed in git as part of a branch name or a reference name.

  • /projects/foo/bar/baz/branches/:/refs/heads/test
  • /projects/foo/bar/baz/branches/~/refs/heads/test

@gregw
Copy link
Contributor

gregw commented Jun 11, 2024

Note that in this case %25 encode is actually allowed by the Servlet Spec, but it is Jetty that is being a bit more cautious. See https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#uri-path-canonicalization

Also, it is possible to build a compliance mode based on other compliance modes, rather than specifically list all of the Violations you wish to allow. In this case, it would probably be simplest to use UriCompliance.from("DEFAULT,AMBIGUOUS_PATH_ENCODING"). Note also that you can just put that in a configuration file like server.ini:

jetty.httpConfig.uriCompliance=DEFAULTjetty.httpConfig.uriCompliance=DEFAULT,AMBIGUOUS_PATH_ENCODING

@davido
Copy link
Author

davido commented Jun 12, 2024

@gregw @joakime

Thanks for clarifying.

I also noticed, that to activate the legacy mode for URL compliance is harder to achieve as is probably should be:

  /**
   * Allow ambiguous path separator and ambiguous path encoding violation.
   *
   * <p><em>WARNING</em> this is not recommended. This is a violation of the Servlet 6 spec rules.
   *
   * @param configuration HTTP configuration instance
   */
  private static void customizeUriCompliance(HttpConfiguration configuration) {
    configuration.setUriCompliance(UriCompliance.LEGACY);
  }

  /**
   * Bypass Servlet 6 spec rules on ambiguous URIs.
   *
   * <p><em>WARNING</em> this is not recommended. This is a violation of the Servlet 6 spec rules.
   *
   * @param httpd Jetty server instance
   * @see <a href="https://github.com/jakartaee/servlet/issues/18">Servlet 6 changes to
   *     getRequestURI / getRequestContextPath / getServletPath / getPathInfo</a>
   */
  private static void bypassServlet6UriRules(Server httpd) {
    httpd
        .getContainedBeans(ServletHandler.class)
        .forEach(handler -> handler.setDecodeAmbiguousURIs(true));
  }

If I already asking the HTTP configuration to set Uri Compliance to UriCompliance.LEGACY,
I wouldn't have expected to ask ServletHandler again, to set decodeAmbiguousURIs property to true.

Can't the latter setting be induced from the former to simplify the client code?

See also the whole changes in JettyServer.java in this CL: [1].

[1] https://gerrit-review.googlesource.com/c/gerrit/+/424580/16/java/com/google/gerrit/pgm/http/jetty/JettyServer.java

@joakime
Copy link
Contributor

joakime commented Jun 12, 2024

If I already asking the HTTP configuration to set Uri Compliance to UriCompliance.LEGACY,
I wouldn't have expected to ask ServletHandler again, to set decodeAmbiguousURIs property to true.

The UriCompliance applies to all features within Jetty 12, and is a URI spec level configuration.
That means jetty-core, jetty-ee8, jetty-ee9, jetty-ee10 (and soon jetty-ee11).
Note that ee8 (Servlet 4) and ee9 (Servlet 5) have no spec mandated restrictions on ambiguous URIs (but those still have the spec bugs regarding ambiguous URIs)

The ServletHandler.setDecodeAmbiguousURIs(boolean) only applies to ee10 (Servlet 6), this flips the Servlet spec level levers.

They are different specs you are controlling.
UriCompliance is for the URI spec.
ServletHandler.setDecodeAmbigiousURIs(boolean) is for the Servlet 6+ spec.

Also keep in mind that if you use anything else in Jakarta EE10, such as Jakarta REST (eg: jaxrs), or Jakarta Pages (eg: JSP), (and many more) you'll have to find the levers for those Spec as well to enable Ambiguous URIs, as all specs in Jakarta are now following the up to date URI spec rules and are not allowing ambiguous path segments for various reasons (a combination of following the servlet spec and various security issues that arise from allowing it in their specs).

@davido
Copy link
Author

davido commented Jun 13, 2024

@joakime Thanks, makes sense.

@prakash-shah
Copy link

prakash-shah commented Aug 22, 2024

I have a java app deployed on appengine using gcloud.
Am unable to figure out where to insert the following code:
httpConfig.setUriCompliance(UriCompliance.from(Set.of(
UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR,
UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING)));

Please advise. I do not have a main (or a Main) in my app.

I am using jetty 12.0.3 in my pom.xml

chia7712 pushed a commit to apache/kafka that referenced this issue Dec 11, 2024
This commit implements the changes for KIP-1032. This updates Kafka to Jakarta specs, JavaEE 10 and Jetty 12. The changes here primarily effect Kafka Connect and MM2.

Todo/Notes:

1) I bumped the connect modules to JDK 17 but I also had to bump a couple other things that had a dependency on conect. The tools project depends on connect so that had to be bumped, and streams depends on tools so that needed to be bumped. This means we may need to separate some things if we don't want to enforce JDK 17 on streams.

2) There is an issue with a test in DedicatedMirrorIntegrationTest that I had to change for now that involves escaping characters and not quite sure what to do about it yet. The cause is the Servlet 6 spec changing what is allowed in the path. See: Jetty 12: 400: Ambiguous URI path encoding for path <%=FOO%>~1 (encoded: %3C%25%3DFOO%25%3E%7E1) jetty/jetty.project#11890

3) I had to configure the idle timeout in Jetty requests to match our request timeout so tests didn't fail. This was needed to fix the ConnectWorkerIntegrationTest#testPollTimeoutExpiry() test

Testing is being done by just using the existing tests for Connect and MM2 which should be sufficient.

Reviewers: Greg Harris <[email protected]>, David Arthur <[email protected]>, Chia-Ping Tsai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

5 participants