Throw exception if URL does not include context path when context relative

[yoshikawaa]

Versions

• Spring 5.2.3
• Spring Security 5.2.1
• Tomcat 9.0.7.B

Problem

#4142
DefaultRedirectStrategy send redirect to "" if redirect argument url not contains application context path.

But on Tomcat, it may not redirect to the context root as intended.
When HttpServletResponse#sendRedirect(""), Tomcat response with empty Location header.

If the request url is /context-root/login and response empty Location header:

Browser	Redierct url	?
Chrome	/context-root/login	NG
Firefox	/context-root/login	NG
IE 11	/context-root	OK

https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletResponse.html#sendRedirect-java.lang.String-

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.

It seems to be the case in the latter case.

Note:
On Weblogic, "" is converted to the context path and set in the Location header.

Solution

Change to send redirect to explicitly specifying the context path.

HttpServletResponse#sendRedirect("")
-> HttpServletResponse#sendRedirect(contextPath)

[eleftherias]

Thanks for the report @yoshikawaa.

Could you explain how you arrived at this behaviour?
What is the scenario in which the application is set to use relative context, but also needs to redirect somewhere outside of the root context?

It would be very helpful if you produce a minimal sample that demonstrates the issue.

[yoshikawaa]

Dear. @eleftherias
I uploaded the sample application.

Could you explain how you arrived at this behaviour?

Please check sample application.
Depending on the browser, "" may be interpreted as the context root or as a relative path from the request. And at least in my environment, Chrome can't interpret "" correctly.

What is the scenario in which the application is set to use relative context, but also needs to redirect somewhere outside of the root context?

When a URL that does not contains the context path is specified, I expected to redirect always to the context root.

... However, if it is determined that "" should indicate a relative path from the request path (the request path itself), then further correction is required.

[eleftherias]

Thank you for the sample @yoshikawaa.

While this sample demonstrates the issue, I am not able to see the cause of it, due to the multitude of dependencies that seem to hide the configuration.

Please modify the sample to confirm to these minimal sample guidelines.

Here are a few key things that are troubling about the sample in its current state:

• You appear to be setting the context-path to redirect-test but I do not see the context set anywhere in the code
• You may be setting tomcat.use-relative-redirects to true but I do not see that setting anywhere in the code
• There are many dependencies in this sample that appear to have side-affects, which makes it impossible to determine the cause of this issue

[yoshikawaa]

@eleftherias
Thank you for the advice.
I reviewed the sample and made it a minimal code.
The code should be easy to understand.
https://github.com/yoshikawaa/spring-security-gh8399

[eleftherias]

Thank you for the updated sample @yoshikawaa.

This scenario occurs when there is a disconnect between what the application is expecting to receive (context relative URL) and what it is actually given (not context relative URL).

When looking at gh-4142, I was hesitant to throw an exception in this case, but now I think that may be the best response.

This situation happens what a developer does something unexpected, not a user, so it may be best to warn the developer that they are doing something that the application is not expecting.

What do you think about throwing an exception in this case, rather than redirecting to the root context path?

[yoshikawaa]

@eleftherias
Looks good.
Considering the role of context relative, I think that it is the correct response to throw an error when a redirect URL outside the context is entered.

Based on this, I will fix this pull request.

[eleftherias]

Thanks for the updates @yoshikawaa!

All of the code looks good.
My one note is to format your commit message to follow our conventions https://github.com/spring-projects/spring-security/blob/master/CONTRIBUTING.adoc#format-commit-messages.

Here is a suggestion, but feel free to update it as you see fit

Throw exception if context path not in redirect

Issue: gh-8399

[yoshikawaa]

@eleftherias
Thanks for your review & advice.
I've kept the message a bit long in order to convey the commit as accurately as possible.

[eleftherias]

Thanks for the PR @yoshikawaa!
This is now merged into master.

I am curious if we have the same issue one the reactive side.
Would you be interested in investigating if DefaultServerRedirectStrategy has the same issue, and if it does, submitting a fix for it?

[yoshikawaa]

Sorry for the late response, @eleftherias .

When using spring-boot-starter-tomcat, reactive's response.getHeaders().setLocation("") produced similar results as servlet's response.sendRedirect("").

spring-security/web/src/main/java/org/springframework/security/web/server/DefaultServerRedirectStrategy.java

Lines 56 to 66 in 28d2cfa

	private URI createLocation(ServerWebExchange exchange, URI location) {
	if (!this.contextRelative) {
	return location;
	}
	String url = location.toASCIIString();
	if (url.startsWith("/")) {
	String context = exchange.getRequest().getPath().contextPath().value();
	return URI.create(context + url);
	}
	return location;
	}

The DefaultServerRedirectStrategy#createLocation method does not perform the process of calculating the redirect destination URL by removing the scheme and context path from the URL.
As a result, It will be response.getHeaders().setLocation("") only when the developer intentionally sets the empty redirect URL (eg URI.create("")).

Since behavior when intentionally specifying an empty URL is as requested by the developer and it is not a trouble of Spring Security, I think that it is no need to fix this.

Regards.

[eleftherias]

Thank you for looking into that @yoshikawaa.
I agree with your assessment.