DelegatingRequestMatcherHeaderWriter with Tiles not work as intended

[yoshikawaa]

Summary

Related #5945

DelegatingRequestMatcherHeaderWriter can not determine the request path exactly as intended.
DelegatingRequestMatcherHeaderWriter determines the request path when committing or including the response, but in that case the request path may have been changed to the JSP path.

My environments as follows.

• Application
  • Spring Boot Dependencies 2.1.1.RELEASE (Spring MVC + Spring Security)
  • Tiles JSP 3.0.8
• Application Server
  • Tomcat 9.0.10 (cargo-maven2-plugin), Tomcat 8.5.9 (Pivotal TC Server 3.2.4)

Please see my sample.

• https://github.com/yoshikawaa/spring-security-gh6338

Actual Behavior

• DelegatingRequestMatcherHeaderWriter#requestMatcher#pattern : /delegating/**.
• The Request Path : /delegating
• The Actual Matching Path : /WEB-INF/views/layout/template.jsp

The request path should be matched and the header should be output.
But in fact, at the timing of DelegatingRequestMatcherHeaderWriter#writeHeaders the request path has already been changed and it will not match.

The problem is that even if this is a problem in implementing an Application server or JSP, it is a possible problem with major servers.

Expected Behavior

It is necessary to be able to match against the actual request path without being influenced by unintended change of the request.

I think that the only correct way to solve the problem is to determine the request path at the timing of HeaderWriterFilter#doFilterInternal.

[jzheaux]

@yoshikawaa Thanks for you post!

There are a couple of ways that I think this could be addressed.

One option you have is that, in a forward, the original request uri is saved in the request as an attribute, which you could obtain like so:

AntPathMatcher pathMatcher = new AntPathMatcher();
RequestMatcher requestMatcher = request -> {
    String uri = (String) request.getAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE);
    return pathMatcher.matches("/delegating/**", uri);
}
return new DelegatingRequestMatcherHeaderWriter(requestMatcher, headerWriter);

A slightly less powerful way is to configure so the headers are written at the beginning of the request: #6501 This can cause problems with the cache control header, so it's not ideal, but might be worth investigating.

It's best if we can remain passive from release to release, and so DelegatingRequestMatcherHeaderWriter would ideally match the same way that it is doing right now.

Feel free to re-open if you feel like there's more to discuss.

[yoshikawaa]

@jzheaux Thanks for your reply and suggestion.

One option you have is that, in a forward, the original request uri is saved in the request as an attribute,

It's best if we can remain passive from release to release, and so DelegatingRequestMatcherHeaderWriter would ideally match the same way that it is doing right now.

I agree to your opinion.
By improving the RequestMatcher, we can solve the problem.
I think that it can be fiexed by modifying AntPathRequestMatcher as follows::

• from

	private String getRequestPath(HttpServletRequest request) {
		if (this.urlPathHelper != null) {
			return this.urlPathHelper.getPathWithinApplication(request);
		}
		String url = request.getServletPath();

		String pathInfo = request.getPathInfo();
		if (pathInfo != null) {
			url = StringUtils.hasLength(url) ? url + pathInfo : pathInfo;
		}

		return url;
	}

• to

	private String getRequestPath(HttpServletRequest request) {
		if (this.urlPathHelper != null) {
			return this.urlPathHelper.getPathWithinApplication(request);
		}

		if (request.getAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE) != null) {
			return constructUrl(
				(String) request.getAttribute(WebUtils.FORWARD_SERVLET_PATH_ATTRIBUTE),
				(String) request.getAttribute(WebUtils.FORWARD_PATH_INFO_ATTRIBUTE));
		}
		
		if (request.getAttribute(WebUtils.INCLUDE_REQUEST_URI_ATTRIBUTE) != null) {
			return constructUrl(
				(String) request.getAttribute(WebUtils.INCLUDE_SERVLET_PATH_ATTRIBUTE),
				(String) request.getAttribute(WebUtils.INCLUDE_PATH_INFO_ATTRIBUTE));
		}
		
		return constructUrl(request.getServletPath(), request.getPathInfo());
	}
	
	private String constructUrl(String url, String pathInfo) {
		if (pathInfo != null) {
			url = StringUtils.hasLength(url) ? url + pathInfo : pathInfo;
		}
		return url;
	}

However, if we really want to match forwarded or included URLs, this may not be appropriate. Such as::

<filter-mapping>
    <filter-name>springSecurityFilterChain</filter-name>
    <url-pattern>/authenticate</url-pattern>
    <dispatcher>FORWARD</dispatcher>
</filter-mapping>

How do you think?

[yoshikawaa]

@jzheaux
jzheaux@05da7bd can also resolve this problem.
I hope that any solution will be applied.

[jzheaux]

@yoshikawaa I also wondered about that and dug into the request matchers a bit to see if modifying them was an option.

The challenge is finding a non-breaking change. The request matchers as well as UrlPathHelper take the stance of getting the request details of the inner-most request context (request, forward, include). You are suggesting logic that elevates the original request as more important, and I don't see any clear evidence that this would be typical across all uses of AntPathRequestMatcher.

Yes, that commit would solve the problem, but it introduces others. We want the headers to be written as late as possible since users have no way to remove them once they are written, and that would generally be just before the response is committed.

One other idea is that you could subclass UrlPathHelper to extract the path in the way that you want, then pass that into the AntPathRequestMatcher constructor.

[yoshikawaa]

@jzheaux I think so, too.

I understand that AntPathRequestMatcher handles the original request or handles the forwarded request in some cases. Changing to always handle the first request path will have a bad influence on other uses.

Certainly, extending pathPathHelper will be resolved this problem.
However, it's nonsense that we must extend whenever we intend to handle the original request path. It is by no means exceptional to forward from one of the controller or View.
If we think of it as a problem with RequestMatcher, I think it would be nice if at least AntPathRequestMatcher has the option of respecting the original request.

But, I felt it necessary to return the discussion to the beginning and to repeat my original opinion.

In the end, the reason that DelegatingRequestMatcherHeaderWriter ( RequestMatcher that it holds) can not properly determine the request path is due to the timing of matching the request path. Writing the header should be done at commit time, but it is too late to perform request path matching at commit time.

I think that the only correct way to solve the problem is to determine the request path at the timing of HeaderWriterFilter#doFilterInternal.

I think it is not reasonable to have to make HeaderWriterFilter to make this correct change.

Regards.