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

Error 400 - Ambiguous URI Empty Segment #11298

Closed
Justvuur opened this issue Jan 22, 2024 · 15 comments · Fixed by #12306
Closed

Error 400 - Ambiguous URI Empty Segment #11298

Justvuur opened this issue Jan 22, 2024 · 15 comments · Fixed by #12306
Assignees
Labels

Comments

@Justvuur
Copy link

Jetty Version
12.0.3

Jetty Environment
ee8

Java Version
JavaSE-17

Question
I just migrated from Jetty 10.0.15 to 12.0.3 and I keep getting the following error:
URI: /badURI
STATUS: 400
MESSAGE: Ambiguous URI empty segment
CAUSED BY: org.eclipse.jetty.http.BadMessageException: 400: Ambiguous URI empty segment

Any ideas what could be causing this?

ambiguous_error

@joakime
Copy link
Contributor

joakime commented Jan 22, 2024

What is your URI?

@lachlan-roberts
Copy link
Contributor

@Justvuur It means your URI has an empty segment (//) which makes it ambiguous. This is because it could be an attempt to bypass some security constraints.

You can set the URI compliance mode in the HttpConfiguration.

See https://eclipse.dev/jetty/javadoc/jetty-12/org/eclipse/jetty/http/UriCompliance.Violation.html#AMBIGUOUS_EMPTY_SEGMENT

When allowing this Violation, the application developer/deployer must ensure that the application behaves as desired when it receives a URI path containing //. Specifically, any URI pattern matching for security concerns needs to be carefully audited.

@yokotaso
Copy link
Contributor

yokotaso commented Feb 5, 2024

If any vaiolation of UriCompliance exists at entry point of ServletContextRequest , Jetty12 using Servlet returns 400 Bad Request.
So, Whether HttpConfiguration is set or not, any violation is not allowed implicitly with using Servlet.
It might be good that UriCompliance check should be taken into account about value of HttpConfiguration#getHttpCompliance

https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java#L195-L208

If you need shorthand workaround, make EmptySegmentHandler and make it handle before ServletContextHandler

public class UriComplianceCheckHandler extends Handler.Wrapper {

    public UriComplianceCheckHandler(Handler handler) {
        super(handler);
    }

    @Override
    public boolean handle(Request request, Response response, Callback callback) throws Exception {
        if (request.getHttpURI().hasViolations()) {
            return super.handle(rewriteRequest(request), response, callback);
        } else {
            return super.handle(request, response, callback);
        }
    }

    Request rewriteRequest(Request request) {
        log.warn("There are HttpURI Violation exists. HttpURI:{}. Violations:{}", request.getHttpURI(),
                request.getHttpURI().getViolations());
        if (request.getHttpURI().hasViolation(UriCompliance.Violation.AMBIGUOUS_EMPTY_SEGMENT)) {
            HttpURI rewrite = rewriteHttpURL(request);
            return Request.serveAs(request, rewrite);
        }
        return request;
    }

    private HttpURI rewriteHttpURL(Request base) {
        String param = base.getHttpURI().getParam();
        String query = base.getHttpURI().getQuery();
        List<String> segments = Arrays
                .stream(base.getHttpURI().getPath().split("\\/"))
                .filter(v -> !v.isEmpty())
                .toList();
        String newPath = "/" + StringUtils.join(segments, "/");

        return HttpURI.build(base.getHttpURI(), newPath, param, query).asImmutable();
    }
}

@Justvuur
Copy link
Author

URI

http://localhost:9998/static/

@Justvuur
Copy link
Author

Ok, I managed to get it working but using http.setUriCompliance(UriCompliance.LEGACY); but I noticed that something somewhere is appending the extra "/". Even though I navigate to "http://localhost:9998/static/", it ends up being "http://localhost:9998/static//".

Any ideas why?

@joakime
Copy link
Contributor

joakime commented Feb 12, 2024

If you see http://localhost:9998/static// with UriCompliance.LEGACY, then that means that is exactly the path as it was sent on the HTTP request from your User-Agent (HTTP Client).
That would also explain the "400: Ambiguous URI empty segment" you were seeing.
That last segment // is valid per URI rules, and it means a segment with no value (hence ambiguous).
It also invalid per Servlet rules, as it cannot match a context-path, url-pattern, or constraint.

To address this problem you have to address what is going on with your HTTP Client.
You can confirm this easily with a network traffic capturing tool like wireshark.

Jetty does not "clean up" these kinds of URIs as there are some libraries that rely on this ambiguity (esp security frameworks).

@salacr
Copy link

salacr commented Sep 22, 2024

I might have similar problem:

Jetty Version
12.0.9

Jetty Environment
ee10

Java Version
JavaSE-17

My request look like this: http://localhost//something.js and I'm also getting 400: Ambiguous URI empty segment

I have set UriCompliance.LEGACY but it doesn't work for me any chance how to get it working?

Thanks!

@gregw
Copy link
Contributor

gregw commented Sep 23, 2024

@salacr Are you sure you have set LEGACY mode?

Note that there is also the CompactPathRule that can be added to the RewriteHandler that will replace '//' with '/'

@salacr
Copy link

salacr commented Sep 23, 2024

@gregw I have unfortunately in ServletContextRequest.java
there is the
// TODO we should check if current compliance mode allows all the violations?

And it still returns the

return new ServletApiRequest.AmbiguousURI(this, msg.toString());

I will check the CompactPathRule then thanks.

@salacr
Copy link

salacr commented Sep 23, 2024

Unfortunately, even when CompactPathRule and the URI are rewritten the violations are already set so I'm still getting same error. Any other advice will be highly apriciated

@chiqiu
Copy link

chiqiu commented Sep 24, 2024

Same here, I changed it to LEGACY in jetty.xml , but when call getSerlvetPath() , it still throws 400 because AmbiguousURI was returned

@joakime
Copy link
Contributor

joakime commented Sep 24, 2024

@chiqiu as pointed out in the Servlet spec, those paths are ambiguous and lead to all kinds of bugs in the servlet spec.
These bugs have been present since the beginning of the Servlet spec (and are not new to ee10 and Servlet 6), and no manner of bug fixing in the spec or the containers can address them properly.
The servlet spec instead decided that these kinds of requests are bad and unsupported.

The AmbiguousURI class properly throws an exception in the case of .getServletPath() and .getPathInfo() as it is not possible to support an ambiguous path with those servlet methods. (there are similar failures for ambiguous paths found in other places in the servlet spec as well. eg: getRequestDispatcher and sendRedirect APIs)

Just using LEGACY without fixing the paths before sending them into the Servlet API will always result in these kinds of issues.

The changes in PR #12306 should help with most, but not all, ambiguous path related fixes that the CompactPathRule is capable of addressing. (other ambiguous path situations need to be addressed with custom code that is specific to your webapp).

@joakime joakime self-assigned this Sep 24, 2024
@gregw
Copy link
Contributor

gregw commented Sep 25, 2024

@gregw I have unfortunately in ServletContextRequest.java there is the // TODO we should check if current compliance mode allows all the violations?

That TODO and that section of the code is all about if ambiguous URIs will be returned from the servlet API getPathInfo and getServletPath methods. By default, even if the compliance mode allows ambiguous URI, those methods will throw if you use them. You need either get the getRequestURI API or call servletHandler.setDecodeAmbiguousURIs(true) so that the servlet API will allow bad URIs to be returned.

I've tested that we do act correctly with the following test added to org.eclipse.jetty.ee10.servlet.ComplianceViolations2616Test

    @Test
    public void testAmbiguousSlash() throws Exception
    {
        String request = """
            GET /dump/foo//bar HTTP/1.1\r
            Host: local\r
            Connection: close\r
            \r
            """;

        String response = connector.getResponse(request);
        assertThat(response, containsString("HTTP/1.1 400 Bad"));

        connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986.with("test", UriCompliance.Violation.AMBIGUOUS_EMPTY_SEGMENT));

        response = connector.getResponse(request);
        assertThat(response, containsString("HTTP/1.1 200 OK"));
    }

@gregw
Copy link
Contributor

gregw commented Sep 25, 2024

I also tested with servletAPI calls in the servlet and used:

    @Test
    public void testAmbiguousSlash() throws Exception
    {
        String request = """
            GET /dump/foo//bar HTTP/1.1\r
            Host: local\r
            Connection: close\r
            \r
            """;

        String response = connector.getResponse(request);
        assertThat(response, containsString("HTTP/1.1 400 Bad"));

        connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.RFC3986.with("test", UriCompliance.Violation.AMBIGUOUS_EMPTY_SEGMENT));
        server.getContainedBeans(ServletHandler.class).stream().findFirst().get().setDecodeAmbiguousURIs(true);

        response = connector.getResponse(request);
        assertThat(response, containsString("HTTP/1.1 200 OK"));
        assertThat(response, containsString("GET /dump/foo//bar"));
    }

gregw added a commit that referenced this issue Sep 25, 2024
@salacr
Copy link

salacr commented Sep 30, 2024

Thanks a lot @gregw that made it working. We will start process of migration to valid urls but it will take quite a time :/ so in meantime this will help a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
7 participants