quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.73k stars 2.67k forks source link

Inconsistent request path matching with X-Forwarded-Prefix #14537

Open kekbur opened 3 years ago

kekbur commented 3 years ago

Describe the bug When X-Forwarded-Prefix header processing is enabled Vertx handlers and Quarkus resources match request paths inconsistently with each other. Specifically, Vertx handlers are matched against the actual request path whereas Quarkus resources annotated with @Path are matched against prefix + actual path.

I believe Vertx handlers exhibit the correct behavior.

Expected behavior Given the Vertx handler

@ApplicationScoped
class VertxHandler
{
    public void register(@Observes Router router)
    {
        router.route("/handler-path").handler(rc -> rc.response()
            .end(rc.request().uri() + "|" + rc.request().absoluteURI())
        );
    }
}

and the Quarkus resource

@Path("/resource-path")
public class QuarkusResource
{
    @Context UriInfo uriInfo;

    @GET
    public String get()
    {
        return uriInfo.getPath() + "|" + uriInfo.getAbsolutePath();
    }
}

requests to /handler-path and /resource-path should yield a 200 - OK response regardless of the value of the X-Forwarded-Prefix header.

Actual behavior A request to /handler-path yields 200 - OK when X-Forwarded-Prefix: /anything. A request to /resource-path yields 404 - Not Found when X-Forwarded-Prefix: /anything. A request to /resource-path yields 200 - OK when X-Forwarded-Prefix: /anything and the resource is configured as @Path("/anything/resource-path").

To Reproduce

Minimal reproducing project.

Steps to reproduce the behavior:

  1. Run the test ForwardedPrefixTest

testVertexHandler succeeds. testQuarkusResource fails.

Configuration

quarkus.http.proxy.proxy-address-forwarding=true
quarkus.http.proxy.enable-forwarded-prefix=true

Environment (please complete the following information):

sberyozkin commented 3 years ago

Indeed in quarkus-oidc we do not expect that X-Forwarded-Prefix is the actual request path segment.

ejba commented 3 years ago

@kekbur I got a 404 in return when I use the prefix and the actual request path. Nevertheless, something is definitely wrong here.

    @Test
    public void testQuarkusResourceWithPrefix() {
        String response = given()
            .header("X-Forwarded-Prefix", "/prefix")
            .when().get("/prefix/resource-path")
            .then()
            .statusCode(HttpStatus.SC_OK)
            .extract().asString();
        assertEquals("/prefix/resource-path|http://localhost:8081/prefix/resource-path", response);
    }
kekbur commented 3 years ago

@ejba

I got a 404 in return when I use the prefix and the actual request path. Nevertheless, something is definitely wrong here.

Oh that's right. I mistakenly wrote:

A request to /anything/resource-path yields 200 - OK when X-Forwarded-Prefix: /anything.

What I should have written is:

A request to /resource-path yields 200 - OK when X-Forwarded-Prefix: /anything and the resource is configured as @Path("/anything/resource-path").

scrocquesel commented 2 years ago

@sberyozkin as per the requirement described here https://github.com/quarkusio/quarkus/issues/9622#issuecomment-643300383, I agree with @atoom.

Current implementation didn't not allow an application to be behind a reverse proxy that does path transformation, like traefik prefix stripping.

If you want some more info on this feature, someone here collects a list of Reverse Proxy or backend framework/libraries supporting it.

We have a lot of microservices exposed on multiple endpoints with different combination of public facing host, port, path prefix. This bug prevent us from migrating to quarkus.

scrocquesel commented 2 years ago

@stuartwdouglas any though on this ? It's been more than a year now that Quarkus first introduces support for this header but since then, it is not really usable.

There is still a very strong coupling between the app and the deployment. It is very hard to deploy quarkus based applications in a path based routing environment and makes quarkus api not really portable.

This pattern is now very common in kubernetes and api gateway. It would be a very important addition to quarkus.

scrocquesel commented 2 years ago

After some more tests with Quarkus 2.5.0.Final, RoutingContext.request().path() and RoutingContext.request().absoluteURI() are also inconsistent when used in a @Route.

path() should be the path part of the absoluteURI() but when requesting

@Route(path = "/foo", methods = HttpMethod.GET )
public String foo(RoutingContext rc) {
    return rc.request().path() + "|" + rc.request().absoluteURI();
}
GET http://localhost:8080/foo
x-forwarded-prefix: /bar

it returns /foo|http://localhost:8080/bar/foo instead of /bar/foo|http://localhost:8080/bar/foo.

This is not the case with the vertx handler above

GET http://localhost:8080/handler-path
x-forwarded-prefix: /bar

as it correctly returns /bar/handler-path|http://localhost:8080/bar/handler-path

stuartwdouglas commented 2 years ago

So the issue here is that we don't really have a 'Match URI' and a 'User Visibile URI', internally we only have one representation, so you can't match on the unprefixed and report the prefixed version.

The inconsistency here comes from the fact that the uri() call is rewritten but not the path, so we do actually have two URI's that are inconsistent with each other (which is a bug), and the difference comes down to which one the frameworks use to match on.

scrocquesel commented 2 years ago

@stuartwdouglas Some framework allows to handle this with a PathBase and a Path properties. Path is used to do routing matching and both are joined for absolutePath.

In normal condition PathBase is empty.

This is also used to allow to route to different inner application. For eg, you may be able to route to an inner router which would be unaware of the PathBase. Say the real request path is /app1/foo, you may be able to route.mapPath("/app1", innerRouter); and the innerRouter will router.route("/foo", ...). In this case, mapPath should just strip the /app1 segment from the Path and add it to the end of pathBase. That way, you could develop a lib with ui routes and another lib with api routes and have both in the same application using mapPath /ui and mapPath /api or use them independently. It's up to the user and the lib developer in unaware of this choice.

antonwiens commented 11 months ago

Any news on this topic? We would like to use this feature with @Path resources. Is it still a problem?

geoand commented 3 weeks ago

Is this still an issue with the latest versions of Quarkus?

kekbur commented 2 weeks ago

Yes, the inconsistent behavior is still there in Quarkus version 3.15.1. Here is an updated reproducer.

geoand commented 2 weeks ago

Thanks

geoand commented 2 weeks ago

The interesting thing is that quarkus-rest behaves in line with what you see in the Vert.x handler.