Hi, let's say that we have a simple architecture with ingress gateway in front that handles all incoming requests, and orders service that is responsible for handling incoming requests about orders domain.

When such request is send to the ingress gateway: GET /api/orders, it should be rerouted to different path, let's say GET /orders. In this case, when orders service returns an error, for example 400 Bad Request, it would contain error details, like:

{
  "timestamp":"2023-09-07T09:29:59.044+00:00",
  "path":"/orders",
  "status": 400,
  "error":"Bad Request",
  "requestId":"31520c96/1-8460"
}

If we take a closer look at path property, it contains wrong path from caller perspective. I wonder if we should add an option, similar to server.error.include-message config property to exclude path from error attributes.

What do you think?

Comment From: wilkinsona

The option to not include the message is for security purposes. That doesn't apply so much to the path. If the gateway set the X-Forwarded-Prefix header and the app was configured to consume it, I think the path in the error response would be correct. Have you got that set up?

If you really want to remove the path attribute, you could provide a custom ErrorAttributes implementation that excludes it. Such an implementation could extend DefaultErrorAttributes and remove the path attribute from the map returned from getErrorAttributes.

Comment From: AleksanderBrzozowski

@wilkinsona Thanks for response πŸ‘

I've just generated a new project with webflux, and tried such a test:

@SpringBootTest(
    webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT,
    properties = [
        "server.port=8080",
        "server.forward-headers-strategy=native"
    ]
)
class HeaderTestApplicationTests {

    private val client = WebTestClient
        .bindToServer()
        .baseUrl("http://localhost:8080")
        .build()

    @Test
    fun `should use X-Forwarded-Prefix path in error`() {
        val response = client.get()
            .uri("/bar")
            .header("X-Forwarded-Prefix", "/foo")
            .exchange()

        response.expectBody()
            .jsonPath("path")
            .isEqualTo("/foo")
    }
}

Unfortunately, it fails, path is equal to /bar. Is it something wrong with the test, or it isn't supported by netty? πŸ™‚

Comment From: quaff

kotlin server.forward-headers-strategy=native

Have you tried with server.forward-headers-strategy=framework ?

Comment From: AleksanderBrzozowski

Yup, I did, it is the same:

    @Test
    fun `should use X-Forwarded-Prefix path in error`() {
        assertEquals(serverProps.forwardHeadersStrategy, FRAMEWORK)

        val response = client.get()
            .uri("/bar")
            .header("X-Forwarded-Prefix", "/foo")
            .exchange()

        response.expectBody()
            .jsonPath("path")
            .isEqualTo("/foo") // reports failure, because it is equal to /bar
    }

Comment From: wilkinsona

To my surprise, ServerRequest uses requestPath().pathWithinApplication().value() which means that the prefix is ignored. That can be changed with custom error attributes:

@Bean
ErrorAttributes customErrorAttributes() {
    return new DefaultErrorAttributes() {

        @Override
        public Map<String, Object> getErrorAttributes(ServerRequest request, ErrorAttributeOptions options) {
            Map<String, Object> attributes = super.getErrorAttributes(request, options);
            attributes.put("path", request.requestPath().value());
            return attributes;
        }

    };
}

In your test, this produces a path of /foo/bar.

I'd like to discuss this with the team as it feels like this would be better default behaviour. However, I may be overlooking something.

Comment From: AleksanderBrzozowski

Oh, yeah, you are right, the path should be /foo/bar πŸ‘

Thanks for your suggestion, we created a similar custom error attributes bean to handle this case πŸ™‚

Comment From: philwebb

We discussed this today on our team call and we're going to look at what happens with the Servlet version so that we can align. We also think adding an exclude option is worthwhile.

Comment From: mhalbritter

I was playing around with that on the Servlet stack. My findings:

  • the native strategy ignores X-Forwarded-Prefix completely, both for errors and for normal dispatches. Tested with Tomcat.
  • the framework strategy works for normal dispatches, request.getRequestURI() returns the path with the X-Forwarded-Prefix prepended
  • the framework strategy doesn't work for error dispatches. The DefaultErrorAttributes uses the jakarta.servlet.error.request_uri attribute, which doesn't take X-Forwarded-Prefix into account.

Here's the project: sb-37269.zip

Comment From: mhalbritter

I've created a ticket for the path inclusion configuration: https://github.com/spring-projects/spring-boot/issues/38619

Comment From: wilkinsona

the framework strategy doesn't work for error dispatches. The DefaultErrorAttributes uses the jakarta.servlet.error.request_uri attribute, which doesn't take X-Forwarded-Prefix into account.

I think this has been addressed by https://github.com/spring-projects/spring-framework/issues/30828

Comment From: mhalbritter

Thanks for the pointer. Just tested with 6.1.2-SNAPSHOT, this resolves the problem when using server.forward-headers-strategy=framework on the servlet stack.

Comment From: mhalbritter

Now that this is fixed on the servlet stack, I think we should change our org.springframework.boot.web.reactive.error.DefaultErrorAttributes to use 

errorAttributes.put("path", request.requestPath().value());

WDYT?

Comment From: wilkinsona

Makes sense to me. I'm not 100% sure on the timing of the change though. It feels like a bug so 3.1.x is tempting but I wonder if there's a chance it'll break someone in which case 3.3 would be better with the option to provide a custom DefaultErrorAttributes extension that uses the old path. I'd prefer 3.1.x if we think it's reasonable.

Comment From: wilkinsona

We discussed this today and decided to address this in 3.3 as the change may not be 100% backwards compatible. In the meantime, those that want the proposed behavior today can use a custom ErrorAttributes bean as shown above.