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 page does not utilize forwarded prefix #30828

Closed
bpfoster opened this issue Jun 22, 2023 · 9 comments
Closed

Error page does not utilize forwarded prefix #30828

bpfoster opened this issue Jun 22, 2023 · 9 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: web-error-handling An issue related to web error handling type: enhancement A general enhancement
Milestone

Comments

@bpfoster
Copy link

Not sure if this belongs in Boot or Framework or somewhere else..

Spring Boot version: 3.0.7

If my Spring Boot application is behind a proxy, and I've configured the X-Forwarded-* headers to be sent, and the Boot application is configured with server.forward-headers-strategy=FRAMEWORK, I would expect the path attribute in the default error response to reflect the forwarded path prefix, but it does not.

I have an extremely simple reproduction application at https://github.com/bpfoster/spring-boot-error-path-forwarded-prefix

Actuator includes the path prefix as expected:

$ curl http://localhost:8081/actuator -H 'X-Forwarded-Prefix: /api' | jq

{
  "_links": {
    "self": {
      "href": "http://localhost:8081/api/actuator",
      "templated": false
    },
    "health": {
      "href": "http://localhost:8081/api/actuator/health",
      "templated": false
    },
    "health-path": {
      "href": "http://localhost:8081/api/actuator/health/{*path}",
      "templated": true
    }
  }
}

The error response does not:

$ curl http://localhost:8081/foobar -H 'X-Forwarded-Prefix: /api' | jq

{
  "timestamp": "2023-06-22T12:29:30.173+00:00",
  "status": 500,
  "error": "Internal Server Error",
  "message": "Foobar",
  "path": "/foobar"
}

Note the hrefs in actuator include the X-Forwarded-Prefix value vs the path in the error do not.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 22, 2023
@wilkinsona
Copy link
Member

wilkinsona commented Jul 5, 2023

Thanks for the sample, @bpfoster. The path in the error page is the value of the jakarta.servlet.error.request_uri request attribute so it would appear that forwarded header handling doesn't affect that attribute. Interestingly, the behavior is the same when using server.forward-headers-strategy=native. I'm not yet sure what this tells us. Things may be working as they should from a Servlet spec perspective or there may be a bug in both Framework and Tomcat.

@wilkinsona
Copy link
Member

Interestingly, the behavior is the same when using server.forward-headers-strategy=native

It's only the behavior on the error-handling side that is the same. The behaviour of the links endpoint changes because the native forwarded header handling in Tomcat (and Jetty and Undertow) does not support X-Forwarded-Prefix so this isn't a Tomcat bug.

When the jakarta.servlet.error.request_uri attribute is being set by Tomcat, the filter chain has complete unwound so the wrapping of the request and response that's done by ForwardedHeaderFilter is gone. This means that there's no opportunity to influence the URI that Tomcat sets as the attribute's value so this isn't a Framework bug either.

That leaves us with doing something in Boot or the application. One possibility is through a custom ErrorAttributes bean:

@Bean
DefaultErrorAttributes errorAttributes() {
	return new DefaultErrorAttributes() {

		@Override
		public Map<String, Object> getErrorAttributes(WebRequest webRequest, ErrorAttributeOptions options) {
			Map<String, Object> errorAttributes = super.getErrorAttributes(webRequest, options);
			addPath(errorAttributes, webRequest);
			return errorAttributes;
		}
		
		private void addPath(Map<String, Object> errorAttributes, WebRequest webRequest) {
			String path = getAttribute(webRequest, RequestDispatcher.ERROR_REQUEST_URI);
			if (path == null) {
				return;
			}
			HttpServletRequest servletRequest = ((ServletWebRequest)webRequest).getRequest();
			while (servletRequest instanceof HttpServletRequestWrapper wrapper) {
				servletRequest = (HttpServletRequest) wrapper.getRequest();
			}
			String pathPrefix = servletRequest.getHeader("X-Forwarded-Prefix");
			errorAttributes.put("path", (pathPrefix != null) ? pathPrefix + path: path);
		}
		
		@SuppressWarnings("unchecked")
		private <T> T getAttribute(RequestAttributes requestAttributes, String name) {
			return (T) requestAttributes.getAttribute(name, RequestAttributes.SCOPE_REQUEST);
		}
		
	};
}

This code assumes that you're using Framework's ForwardedHeaderFilter and, therefore, that X-Forwarded-Prefix will have been honored everywhere else. I'm not sure how easy it would be for us to detect this reliably in Boot's own code such that a change could be made to DefaultErrorAttributes itself. As such, this may just have to be worked around in application code.

I'll label the issue for discussion in a team meeting so that we can consider our options.

@wilkinsona wilkinsona removed their assignment Jul 6, 2023
@bpfoster
Copy link
Author

bpfoster commented Jul 6, 2023

Thanks for the response @wilkinsona! Tomcat not using X-Forwarded-Prefix was my understanding as well. X-Forwarded-For, -Host and -Proto appear to be fairly standard but -Prefix seems to be a more unique header, and is the main reason we are using the Framework forward-headers-strategy.

I do notice in DefaultErrorAttributes, that webRequest.getRequest() is an instance of ForwardedHeaderFilter$ForwardedHeaderExtractingRequest, so perhaps there is some way to make that detection that ForwardedHeaderFilter is in use.

I would think, if possible, it'd be better to have boot/framework automatically make this decoration in DefaultErrorAttributes (or elsewhere) rather than needing to do this in individual application code.

In the end, the path in the error response is probably not a big deal, but I did expect to see a consistent representation between these paths and that the error representation generated by Spring would have the same path handling as other points.

@wilkinsona
Copy link
Member

We talked about this today and realised that this could be addressed in Framework. When ForwardedHeaderFilter is called during the error dispatch, it could apply the value of the X-Forwarded-Prefix header to the value of the jakarta.servlet.error.request_uri request attribute. We'll transfer this to the Framework team so that they can consider such a change.

@bclozel bclozel transferred this issue from spring-projects/spring-boot Jul 6, 2023
@bclozel bclozel added in: web Issues in web modules (web, webmvc, webflux, websocket) theme: web-error-handling An issue related to web error handling labels Jul 6, 2023
@rstoyanchev
Copy link
Contributor

We could set the jakarta.servlet.error.request_uri request attribute, but ideally should also restore it. This could be done from doFilter, but I see we sometimes recalculate from within calls to request.getRequestURI() in which case there is no good place to set and then restore.

I'm wondering if Boot could rely on request.getRequestURI() instead of the attribute?

@wilkinsona
Copy link
Member

wilkinsona commented Nov 8, 2023

Won't request.getRequestURI() return the URI for the forward to the error page? We need the URI for the original request that triggered the error. AFAIK, the jakarta.servlet.error.request_uri request attribute is the only way to get that.

@rstoyanchev rstoyanchev self-assigned this Nov 13, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 13, 2023
@rstoyanchev rstoyanchev added this to the 6.1.0 milestone Nov 13, 2023
@rstoyanchev
Copy link
Contributor

Indeed, I'll give it a try.

@rstoyanchev rstoyanchev modified the milestones: 6.1.0, 6.1.1, 6.1.2 Nov 16, 2023
rstoyanchev added a commit that referenced this issue Nov 30, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 30, 2023

Thanks for the sample @bpfoster. There is now a fix, which I have confirmed with the sample, but if you'd like to give it a try in your application, please use 6.1.2-SNAPSHOT.

@bpfoster
Copy link
Author

Hi @rstoyanchev, sorry for the delayed response. We've upgraded to framework 6.1.3 and I can confirm that I now see the path prefix in the error path. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) theme: web-error-handling An issue related to web error handling type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants