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

Add 'server.remove-forward-headers' options #11525

Closed
bclozel opened this issue Jan 5, 2018 · 8 comments
Closed

Add 'server.remove-forward-headers' options #11525

bclozel opened this issue Jan 5, 2018 · 8 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement

Comments

@bclozel
Copy link
Member

bclozel commented Jan 5, 2018

Enabling that option should create a ForwardedHeaderFilter option and remove Forwarded headers.

This is especially useful if your application is looking at Forwarded headers even though it's not protected by a proxy (adding/removing those headers already).

See Rossen's comment in #10900 (comment) for more background about this.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 5, 2018

An additional question is whether this property should be set by default. Doing so would cause applications that actually need to use forwarded headers to fail but the failure cause should be fairly obvious. The benefit is that it leads down a safer path of explicitly opting into the use of forwarded headers. Note also that setting the flag by default has the potential and likely will cause regression in applications that currently rely on forwarded headers implicitly, i.e. by virtue of using MvcUriComponentsBuilder or ServletUriComponentsBuilder to create links.

@bclozel
Copy link
Member Author

bclozel commented Jan 5, 2018

One thing bothers me, though. If that's really insecure and not obvious, why is that the default in Spring Framework? Shouldn't ServletUriComponentsBuilder and MvcUriComponentsBuilder be safe by default?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 8, 2018

That's just it though, there is no way to toggle this on and off in the Spring Framework. The only option would be to stop consuming those headers.

@rstoyanchev
Copy link
Contributor

The only option would be to stop consuming those headers.

And that's what we intend to do in 5.1 https://jira.spring.io/browse/SPR-16668.

@wilkinsona wilkinsona modified the milestones: 2.1.0, Backlog Mar 29, 2018
@bclozel bclozel modified the milestones: Backlog, Icebox Jun 1, 2018
@sangeetha5491
Copy link

Is this still an active issue that needs to be worked on?

@wilkinsona
Copy link
Member

@sangeetha5491 Yes, although I don't think we know exactly what form that work should take just yet.

@wilkinsona wilkinsona added the status: pending-design-work Needs design work before any code can be developed label Dec 7, 2018
@bclozel
Copy link
Member Author

bclozel commented Dec 7, 2018

Since SPR-16668, Spring applications don't rely on forwaded headers by default. As explained in the Spring Framework migration docs, you either need to enable the forwarded headers support at the server level (which is what Spring Boot provides) or configure a ForwardedHeaderFilter.

If we provide a server.remove-forward-headers that configures a ForwardedHeaderFilter to remove all related headers, then we'd have two problems:

  1. This might confuse developers using the forwarded headers support at the server level with Spring Boot; I think the server support should operate before the Servlet filter, making that filter a no-op?
  2. Removing such headers might not help anymore since nothing consumes them in Spring at that point

With that in mind, I think we should close this issue. If developers need to remove those headers for a specific reason, then creating a ForwardedHeaderFilter bean and configuring it is perfectly fine.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Dec 7, 2018
@bclozel bclozel added for: team-meeting and removed for: team-attention An issue we'd like other members of the team to review labels Dec 7, 2018
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-meeting status: pending-design-work Needs design work before any code can be developed labels Jan 16, 2019
@bclozel
Copy link
Member Author

bclozel commented Jan 16, 2019

The team discussed this issue and we think that this is not useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants