Use request.requestPath().value() to populate path error attribute with WebFlux

[AleksanderBrzozowski]

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?

[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.

[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? 🙂

[quaff]

server.forward-headers-strategy=native

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

[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
    }

[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.

[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 🙂

[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.

[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

[mhalbritter]

I've created a ticket for the path inclusion configuration: #38619

[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 spring-projects/spring-framework#30828

[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.

[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?

[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.

[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.