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

ForwardedHeaderFilter should expose option for not converting relative redirects to absolute ones [SPR-15717] #20273

Closed
spring-issuemaster opened this issue Jun 29, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jun 29, 2017

Eric Sirianni opened SPR-15717 and commented

Upon upgrade to Spring 4.3.5, all of our application redirects changed from relative to absolute.

The cause is this code added for #19587 converts a relative redirect to an absolute one:

			// Relative to Servlet container root or to current request
			String path = (location.startsWith(FOLDER_SEPARATOR) ? location :
					StringUtils.applyRelativePath(this.request.getRequestURI(), location));

			String result = UriComponentsBuilder
					.fromHttpRequest(new ServletServerHttpRequest(this.request))
					.replacePath(path)
					.build().normalize().toUriString();

I don't understand why this is necessary or appropriate. Won't a relative redirect already be interpreted relative to the client-facing view of the scheme/host/part? Why the need to transform it into an absolute one?

Can this be parameterized so that this functionality can be disabled if not desired?


Affects: 4.3.5

Issue Links:

  • #19587 ForwardedHeaderFilter should support sendRedirect

Referenced from: commits 68e6b14, 4160ced, 5f868b4

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2017

Rossen Stoyanchev commented

I think it's probably due to this from the Javadoc of sendRedirect which means the client wouldn't actually get a relative URL anyway:

This method can accept relative URLs;the servlet container must convert the relative URL to an absolute URL before sending the response to the client.

Rob Winch what's your take on this based on your experiments under #19587?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2017

Eric Sirianni commented

Yes, that is likely based on the old RFC which required redirects to be absolute. Tomcat (for one) exposes an option to override this behavior useRelativeRedirects:

From https://tomcat.apache.org/tomcat-8.0-doc/config/context.html :

Controls whether HTTP 1.1 and later location headers generated by a call to javax.servlet.http.HttpServletResponse#sendRedirect(String) will use relative or absolute redirects. Relative redirects are more efficient but may not work with reverse proxies that change the context path. It should be noted that it is not recommended to use a reverse proxy to change the context path because of the multiple issues it creates. Absolute redirects should work with reverse proxies that change the context path but may cause issues with the org.apache.catalina.filters.RemoteIpFilter if the filter is changing the scheme and/or port. If the org.apache.catalina.STRICT_SERVLET_COMPLIANCE system property is set to true, the default value of this attribute will be false, else the default value will be true.

Ideally, this setting could be auto-detected by ForwardedHeaderFilter and it could act accordingly. However, this approach seems fragile and the container-specific. So it seems simpler to just allow the user of ForwardedHeaderFilter to manually configure this behavior.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 7, 2017

Rob Winch commented

Eric Sirianni Can you please describe the problem that having an absolute URL is causing you?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2017

Eric Sirianni commented

@rwinch In our particular case, we are seeing issues with a hardware load balancer that is also doing SSL termination. It seems to be incorrectly rewriting Location header in responses, but only when there is an absolute redirect. We have been using relative redirects as a workaround. In general, it seems safer to rely on relative redirects rather than relying on every proxy in the chain to properly deal with Forwarded headers.

You may wonder why we need ForwardedHeaderFilter at all if we are using relative redirects. We still need it with a Spring Cloud Netflix Zuul proxy setup in order to get the X-Forwarded-Prefix set properly.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 14, 2017

Rob Winch commented

Eric Sirianni Thanks for the response. I'm wanting to make sure I understand your scenario entirely.

Are you asking that ForwardedHeaderFilter rewrite the sendRedirectLocation but just be a relative URL or are you asking ForwardedHeaderFilter to do nothing (just pass the value as is to the Servlet Container) when using sendRedirectLocation?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 14, 2017

Eric Sirianni commented

Are you asking that ForwardedHeaderFilter rewrite the sendRedirectLocation but just be a relative URL or are you asking ForwardedHeaderFilter to do nothing (just pass the value as is to the Servlet Container) when using sendRedirectLocation?

Based on my understanding both approaches would be acceptable. Since the ForwardedHeaderFilter overrides getContextPath() to accommodate X-Forwarded-Prefix, client code should already be building "forwarding aware" redirects anyway. So explicit rewriting of the path component of the URL in sendRedirectLocation should not be needed.

The only time ForwardedHeaderFilter would need to get involved is if the Servlet Container itself is converting a relative redirect to an absolute one in a way that does not respect the X-Forwarded headers. In this case the ForwardedHeaderFilter would need to proactively make the URL absolute to avoid the Container's incorrect conversion to an absolute URL. This seems to be the motivation behind #19587.

This is why I think the behavior needs to be configurable - ForwardedHeaderFilter has no way of knowing if a relative URL is "safe" to pass up to the Container's super.sendRedirect() or not. This is dependent on whether or not the developer has configured the Container to use relative redirects.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 17, 2017

Rob Winch commented

Eric Sirianni Thanks for you response.

I'd like to take a slight variation on your proposal. What if ForwardedHeaderFilter provided a flag that when enabled overrode the HttpServletResponse.sendRedirect to instead set the Location header with the location passed into sendRedirect and the HTTP Status code directly. This would ensure that relative redirects would work independent of the Servlet Container settings.

Would this work for your needs?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 18, 2017

Rob Winch commented

Eric Sirianni I have been discussing this with Rossen Stoyanchev a bit and we are reconsidering options at the moment.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2017

Juergen Hoeller commented

Alright, I'll do this backport tonight, along with another that I got left on my side...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.