DefaultRedirectStrategy should not calculate relative url if it does not contain the context-path

[mpalourdio]

Hello,

When defining a redirect strategy, after a logout for example, you can force the redirection to be context relative, by setContextRelative(true).

If by mistake (or not), you want the redirect target to be an absolute url completely outside of your context-path (eg: http://www.google.com), this url obviously does not contain your context-path.

This lead to DefaultRedirectStrategy#calculateRedirectUrl silently calculating a completely wrong URL if contextRelative is true

This PR checks is the URL contains the context-path. If not, it early returns the untouched url.

Another possibility would be to throw an IllegalArgumentException if contextRelative is true BUT url is absolute AND does not contain the context-path.

[pivotal-issuemaster]

@mpalourdio Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@mpalourdio Thank you for signing the Contributor License Agreement!

[eleftherias]

Thanks for the PR @mpalourdio!
I agree that this situation leads to unexpected behaviour, but I don't think returning the full URL is the right solution.
According to the Javadoc of the sendRedirect method: If contextRelative is set, the redirect value will be the value after the request context path.
In the case where the redirect URL does not contain the context path, it may be reasonable to return the empty String ("").
Does that seem like sensible behaviour to you?

[mpalourdio]

@eleftherias Thanks for feedback. I haven't really thought about this, it's been a while now :)

But looking at the fact that you would prefer returning an empty string, here is how it would look

if (isContextRelative() && !url.contains(contextPath)) {
     return "";
}

Which would lead to sendRedirect(""); Which is bad too IMO.

As a security library, I finally think that it simply should fail by throwing an exception.
Anyway, the decision is yours, and I can adapt the PR if still needed.

Otherwise, feel free to close this issue.

[eleftherias]

@mpalourdio My concern with throwing an exception in the DefaultRedirectStrategy class is that the end user will end up on the generic browser error page with a 500 status code, and they won't know how to proceed.
For example an IllegalArgumentException won't be handled anywhere down the line and will result in a 500 status code.

My thought with returning "" is that users will still remain within the application (since they will be redirected to the base context), rather than being redirected to an external URL that the application did not intend.

Let me know if you think there is a better approach, or an exception that is better suited than IllegalArgumentException.

[mpalourdio]

@eleftherias I have updated the PR so that it redirects to "" when needed.

[eleftherias]

@mpalourdio Thank you for the quick turnaround. One last request is to change the URL in the test from "http://redirectme.somewhere.else" to "https://redirectme.somewhere.else". We avoid using "http://" in our code and use the nohttp library to find occurrences. You can read more about it here https://github.com/spring-io/nohttp.

[mpalourdio]

@eleftherias done :)

[eleftherias]

Thanks for all your work @mpalourdio! This is now merged into master.