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 support sendRedirect [SPR-15020] #19587

Closed
spring-projects-issues opened this issue Dec 14, 2016 · 11 comments
Closed

ForwardedHeaderFilter should support sendRedirect [SPR-15020] #19587

spring-projects-issues opened this issue Dec 14, 2016 · 11 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 14, 2016

Swagata Roy opened SPR-15020 and commented

By default, Spring Boot provides an embedded Apache Tomcat build. Unfortunately, Tomcat does not support X-Forwarded-Host.

Here is the redirect code -

@RequestMapping("/testRedirect")
  public String redirect() {
      return "redirect:/home";
  }

These are the results -

  curl -i -H "X-Forwarded-Proto:https" -H "X-Forwarded-Host:hare2" http://localhost:8080/testRedirect
HTTP/1.1 302
Location: http://localhost:8080/home
Content-Language: en-US
Content-Length: 0
Date: Wed, 14 Dec 2016 16:25:36 GMT

This is what I am expecting -

curl -i -H "X-Forwarded-Proto:https" -H "X-Forwarded-Host:hare2" http://localhost:8080/testRedirect
HTTP/1.1 302
Location: https://hare2/home
Content-Language: en-US
Content-Length: 0
Date: Wed, 14 Dec 2016 17:53:21 GMT

Affects: 4.3.4

Attachments:

Issue Links:

  • #20273 ForwardedHeaderFilter should expose option for not converting relative redirects to absolute ones

Referenced from: pull request #1270

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 14, 2016

Swagata Roy commented

I was able to get this working by using the attached code.

Thanks!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 15, 2016

Rossen Stoyanchev commented

Tomcat has a RemoteIpFilter that also seems to only take care of the request. I don't see anything for redirects.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 15, 2016

Rob Winch commented

Rossen Stoyanchev It probably should have something for redirects. The Javadoc on sendRedirect states:

... . 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. If the location is relative without a leading '/' the container interprets it as relative to the current request URI. If the location is relative with a leading '/' the container interprets it as relative to the servlet container root.

Another thing to point out is that many people probably use the RemoteIpValve which rewrites the catalina Request which is used by the catalina Response to sendRedirect. In short, the RemoteIpValve only needs to change the request and this automatically ensures HttpServletResponse.sendRedirect(String) works properly.

You can see an example of this working in https://github.com/rwinch/boot-forwardedheaderfilter/tree/remoteipvalve You can see how the scheme is overridden

You can also see that Jetty supports overriding the values using ForwardedRequestCustomizer. I added a branch with tests that demonstrate how Jetty behaves a bit different. It does not work properly for the // URLs. Additionally, Jetty supports the X-Forwarded-Host which Tomcat does not as discussed in spring-projects/spring-boot#5677. The locationXForwardedLogin test demonstrates this.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 23, 2016

Stefan Pfeiffer commented

This change somehow conflicts with DefaultRedirectStrategy.sendRedirect(), which prepends request.getContextPath() to absolute URLs before calling response.sendRedirect(). ForwardedHeaderFilter.sendRedirect() prepends the context path again. A redirect to `` finally ends up at \contextpath\contextpath. I do not know where to fix that problem proper, but perhaps can one of you look into the issue? Thanks.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 23, 2016

Rob Winch commented

Is `` a typo? You should be using /. Can you try that or confirm the typo? If there is still an issue, please create a new ticket and link to this one.

Thanks!
Rob

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 23, 2016

Swagata Roy commented

I have tested, this works for me using 4.3.5. Also, we have set server.use-forward-headers=false just to make sure this doesn't have get activated by accident as the ForwardedHeaderFilter is doing all the work for us.

Thanks!
Swagata

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 23, 2016

Stefan Pfeiffer commented

@Rob Winch Yes, that was a typo, sorry. I meant "/". Any URL starting with "/" should trigger the duplicate prepending of the contextpath (one by https://github.com/spring-projects/spring-security/blob/master/web/src/main/java/org/springframework/security/web/DefaultRedirectStrategy.java#L66 and the other one by https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/filter/ForwardedHeaderFilter.java#L265). You just need a setting where DefaultRedirectStrategy kicks in, sees a target-URL starting with "/" and does a Response.sendDirect(…) that somewhere along the chain of response wrappers ends up in ForwardedHeaderFilter. Over the holidays i will try and isolate the problem in a small demo project and open another ticket. My observation comes from stepping through the code in a complex project and looking at the code i see.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 23, 2016

Stefan Pfeiffer commented

@sroy Wait. I have to set server.use-forward-headers=false now to make forward-headers work correctly? Counter-intuitive...I do not see where this parameter would kick in along the code path i was looking at in the debugger, but in my case, we certainly had server.use-forward-headers=true set to make stuff work pre-4.3.5. I will try again and report back. Thanks so far!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 24, 2016

Rob Winch commented

Stefan Pfeiffer By default Spring Boot uses built in x-forwarded support which is servlet container specific. The ForwardedHeaderFilter is only used if you explicitly configure it. If you are using ForwardedHeaderFilter it is best to disable the container specific support using server.use-forward-headers=false because it may interfere with ForwardedHeaderFilter in some circumstances.

Also keep in mind that these changes will only be seen if you are using Spring 4.3.5 which is not included by default in Spring Boot until 1.4.3.

I have put together a small Spring Boot sample with some tests that appear to demonstrate things work properly with Spring Security's DefaultRedirectStrategy. See DemoApplicationTests.

If none of the information above helps and you still think there is a problem, please feel free to use the above sample as a starting point for recreating the issue you have. Once you can recreate the issue (or provide us information on how to create it), please create a ticket and reference it here in the comments.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 3, 2017

Stefan Pfeiffer commented

I submitted a PR to the base example that shows the behaviour described in #19654 and makes the tests fail.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 3, 2017

Stefan Pfeiffer commented

As a side note, the problem is completely unaffected by the server.use-forward-headers property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.