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

server.tomcat.use-relative-redirects=true not honored when server.forward-headers-strategy=framework #29333

Closed
wants to merge 1 commit into from

Conversation

terminux
Copy link
Contributor

Fixes gh-27801

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 12, 2022
@terminux terminux changed the base branch from main to 2.5.x January 12, 2022 11:56
@wilkinsona
Copy link
Member

Thanks for the proposal, @terminux, but I think it may be a little too broad. I believe we should only configure the filter to use relative redirects when Tomcat's in use and it has been configured to use relative redirects. If another container's in use or Tomcat's using absolute redirects, the filter's configuration should be unchanged.

Unfortunately, I haven't had time to verify that what I believe is required actually is required or to think about how best to implement it. If you have time to do so, that'd be great. Please don't worry if you don't though and thanks again for your efforts thus far.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 12, 2022
@terminux
Copy link
Contributor Author

Thank you very much for your reply @wilkinsona , nice suggestions. I will reconsider how to implement it based on your suggestion.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 12, 2022
@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 12, 2022
@terminux terminux changed the title Relative redirects is automatically enabled when server.forward-headers-strategy is configured as framework Fix the bug that the use-relative-redirects configuration item of tomcat does not take effect Jan 13, 2022
@terminux
Copy link
Contributor Author

Hi @wilkinsona , my idea is to add a static config class specifically for ForwardedHeaderFilter and we don't automatically enable relative redirects for tomcat containers, only if server.tomcat.use-relative-redirects is configured to true, It might be better. I updated the PR and it's ready for your review again.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 13, 2022
@wilkinsona
Copy link
Member

Thanks, @terminux. When initially thinking about this, I had rejected something along these lines as I didn't want the configuration of the ForwardedHeaderFilter to know about Tomcat directly. However, having seen your proposal it's quite an elegant and pragmatic solution. It does need to be refined a little as there's no guarantee about the processing order of the two @Bean methods for the filter. The ordering can be controlled by putting each method in a separate class and @Importing those two classes in the desired order.

The alternative to coupling to ForwardedHeaderFilter configuration directly to Tomcat would be a general callback interface for customizing the ForwardedHeaderFilter and a Tomcat-specific implementation of it. That would remove the need for multiple @Bean methods, but would require the new callback interface.

I'll flag this for a team meeting so that we can discuss these options and any others that come to mind.

@wilkinsona wilkinsona added for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: feedback-provided Feedback has been provided labels Jan 18, 2022
@terminux
Copy link
Contributor Author

Thanks @wilkinsona , I originally thought about using @Order to control the processing order of two @Bean methods, but it's less elegant. However, when I saw your comment, I found that @Import is indeed a good choice !

I also thought about using the callback interface way, for example:

interface ForwardedHeaderFilterCustomizer {

	void customize(ForwardedHeaderFilter filter);

}

But actually ForwardedHeaderFilter has only two methods (setRemoveOnly and setRelativeRedirects) that may be called. I'm not sure if adding a new callback interface for this would be a bit wasteful, but it scales well.

@philwebb philwebb added for: merge-with-amendments Needs some changes when we merge type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Feb 2, 2022
@philwebb philwebb added this to the 2.5.x milestone Feb 2, 2022
@wilkinsona wilkinsona changed the title Fix the bug that the use-relative-redirects configuration item of tomcat does not take effect server.tomcat.use-relative-redirects=true not honored when server.forward-headers-strategy=framework Feb 3, 2022
wilkinsona pushed a commit that referenced this pull request Feb 10, 2022
Previously, when Tomcat was configured to use relative redirects
and the ForwardedHeaderFilter is in use, the filter would ignore
the use of the relative redirects.

This commit corrects this misalignment by applying Tomcat's use
relative redirects setting to the filter, but only when Tomcat is
being used as the servlet container.

See gh-29333
@wilkinsona
Copy link
Member

Thanks very much, @terminux. I went for a customizer callback in the end, but as in implementation detail that isn't part of Boot's public API.

@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.5.10 Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

server.tomcat.use-relative-redirects=true not honored when server.forward-headers-strategy=framework
5 participants