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

Consider using ForwardedHeaderFilter #5677

Closed
rwinch opened this issue Apr 13, 2016 · 10 comments
Closed

Consider using ForwardedHeaderFilter #5677

rwinch opened this issue Apr 13, 2016 · 10 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@rwinch
Copy link
Member

rwinch commented Apr 13, 2016

Spring 4.3 added ForwardedHeaderFilter which provides a cross container independent way to support X-Forwarded-* headers.

This is nice because not all containers provide the same support. For example, Tomcat does not support X-Forwarded-Host. Another benefit is that ForwardedHeaderFilter allows overriding the context root of the application in the event the proxy uses a different context root than the application. This is helpful to ensure links are rendered correctly.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 13, 2016
@philwebb
Copy link
Member

I'm not sure that we should change the current server.use-forward-headers property to use the filter. Since it's currently supported for Tomcat, Undertow and Jetty I'd rather just see the Tomcat bug fixed.

@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 15, 2016
@rwinch
Copy link
Member Author

rwinch commented Apr 15, 2016

@philwebb Thanks for the feedback. I think there are a few advantages to using Spring's ForwardedHeaderFilter:

  • You can support overriding the context root. I have seen this requirement for quite a few Pivotal Labs clients. (I have also experienced this need in the past)
  • It will be available right away
  • I'm guessing it will simplify the codebase since you can do the same thing for every container rather than having to adapt to each container specific implementation

@philwebb
Copy link
Member

@rwinch Are there any downsides? Filter ordering for example? or performance?

@rwinch
Copy link
Member Author

rwinch commented Apr 15, 2016

I doubt that performance would be an issue.

The only limitation I can think of is that a Filter would not wrap a ServletRequestListener. However, we already have lots of limitations around listeners.

@wilkinsona
Copy link
Member

wilkinsona commented Apr 23, 2016

I am against this, for now at least. When Undertow's native support is used, the HttpServerExchange is marked as secure which has the knock-on effect of marking the request as secure. Anything in Undertow that looks at the exchange will get the wrong answer if we move to the filter-based approach.

@rwinch
Copy link
Member Author

rwinch commented Apr 25, 2016

@wilkinsona What is looking at the HttpExchange vs the HttpServletRequest (which is correctly overridden)?

@wilkinsona
Copy link
Member

Nothing in Undertow itself, other than its MarkSecureHandler which marks every request as secure by configuring an attachment on the exchange. The problem is that we have no way of knowing what other code might be looking in the "wrong" place. For that code, moving to ForwardedHeaderFilter would be a breaking change.

@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review labels May 19, 2016
@philwebb
Copy link
Member

Sorry, but we feel the current approach is probably better overall.

@kakawait
Copy link
Contributor

kakawait commented Jun 16, 2016

Why not adding server.use-forward-headers-strategy with Enum (can find better name for value) like NATIVE (or CONTAINER), FILTER? As @rwinch explain ForwardedHeaderFilter can be useful for some use cases.

@bclozel
Copy link
Member

bclozel commented Jan 16, 2019

We've decided to reconsider this option and allow this use case with a enum, as suggested by @kakawait.
We should probably deprecate the existing server.use-forward-headers option and only use the enum option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants