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

HandlerMappingIntrospector does not work with PathPattern backed HandlerMappings #26814

Closed
philwebb opened this issue Apr 16, 2021 · 3 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@philwebb
Copy link
Member

Discovered whilst working on spring-projects/spring-boot#24645. Running CorsSampleActuatorApplicationTests.preflightRequestToEndpointShouldReturnOk from this branch should replicate the problem.

This issue is a quite subtle and hard to replicate. I've found it to cause problems for CorsFilter as well as MvcRequestMatcher in Spring Security. It appears that HandlerMappingIntrospector can fail to find mappings if they are configured with a PathPattern.

In AbstractHandlerMapping.initLookupPath there's the following branch:

if (usesPathPatterns()) {
	request.removeAttribute(UrlPathHelper.PATH_ATTRIBUTE);
	RequestPath requestPath = ServletRequestPathUtils.getParsedRequestPath(request);
	String lookupPath = requestPath.pathWithinApplication().value();
	return UrlPathHelper.defaultInstance.removeSemicolonContent(lookupPath);
}
else {
	return getUrlPathHelper().resolveAndCacheLookupPath(request);
}

This means that ServletRequestPathUtils.getParsedRequestPath(request) is called when a PathPattern is set. That code will fail if request.getAttribute(PATH_ATTRIBUTE) is null.

Usually HandlerMappers are only called from the DispatcherServlet which has the following logic:

RequestPath previousRequestPath = null;
if (this.parseRequestPath) {
	previousRequestPath = (RequestPath) request.getAttribute(ServletRequestPathUtils.PATH_ATTRIBUTE);
	ServletRequestPathUtils.parseAndCache(request);
}

The problem is that HandlerMappingIntrospector is designed to be called from a Filter which means that it can be executed before the DisatcherServlet runs.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 16, 2021
@rstoyanchev rstoyanchev self-assigned this Apr 16, 2021
@rstoyanchev rstoyanchev added this to the 5.3.7 milestone Apr 16, 2021
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 16, 2021
@rstoyanchev
Copy link
Contributor

It looks like the original intent was that Spring Security might configure ServletRequestPathFilter in its filter chain. However, when the solution was refined so that its Spring Security can remain unaware of the kind of patterns used, the HandlerMappingIntrospector was never updated. I will fix that.

I'm not sure what the issue is for CorsFilter, I presume that is a variation of the same issue when Spring Security calls HandlerMappingIntrospector#getCorsConfiguration?

@philwebb
Copy link
Member Author

Yes, it's this line when configSource is a HandlerMappingIntrospector. The CorsConfiguration is never returned because this line catches and swallows this exception.

@rstoyanchev
Copy link
Contributor

Okay thanks for confirming.

odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue May 7, 2021
…correctly.

We now also implement MatchableHandlerMapping.getPatternParser() to expose the PathPatternParser we use. This is needed to detect that DelegatingHandlerMapping uses a PathPatternParser from the outside as the Spring MVC request processing pipeline treats those HandlerMappings in a special way. This is especially important in the context of spring-projects/spring-framework#26814 as that makes the HM implementations work properly in CORS preflight requests.

When used with Spring Security in place, CORS preflight requests are inspected through the HandlerMappingIntrospector, that looks up the CORS configuration for the handler to serve the actual request. That lookup previously failed as Spring MVC was unable to detect that Spring Data REST's DelegatingHandlerMapping works with a PathPatternParser and didn't properly set up the processing pipeline to eventually end up in an invalid state, produce an exception that caused the handler method lookup to fail and cause Spring Security to fail with a 403, claiming the request was invalid.
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue May 7, 2021
…correctly.

We now also implement MatchableHandlerMapping.getPatternParser() to expose the PathPatternParser we use. This is needed to detect that DelegatingHandlerMapping uses a PathPatternParser from the outside as the Spring MVC request processing pipeline treats those HandlerMappings in a special way. This is especially important in the context of spring-projects/spring-framework#26814 as that makes the HM implementations work properly in CORS preflight requests.

When used with Spring Security in place, CORS preflight requests are inspected through the HandlerMappingIntrospector, that looks up the CORS configuration for the handler to serve the actual request. That lookup previously failed as Spring MVC was unable to detect that Spring Data REST's DelegatingHandlerMapping works with a PathPatternParser and didn't properly set up the processing pipeline to eventually end up in an invalid state, produce an exception that caused the handler method lookup to fail and cause Spring Security to fail with a 403, claiming the request was invalid.
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue May 7, 2021
…correctly.

We now also implement MatchableHandlerMapping.getPatternParser() to expose the PathPatternParser we use. This is needed to detect that DelegatingHandlerMapping uses a PathPatternParser from the outside as the Spring MVC request processing pipeline treats those HandlerMappings in a special way. This is especially important in the context of spring-projects/spring-framework#26814 as that makes the HM implementations work properly in CORS preflight requests.

When used with Spring Security in place, CORS preflight requests are inspected through the HandlerMappingIntrospector, that looks up the CORS configuration for the handler to serve the actual request. That lookup previously failed as Spring MVC was unable to detect that Spring Data REST's DelegatingHandlerMapping works with a PathPatternParser and didn't properly set up the processing pipeline to eventually end up in an invalid state, produce an exception that caused the handler method lookup to fail and cause Spring Security to fail with a 403, claiming the request was invalid.

Fixes #2007.
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue May 7, 2021
…correctly.

We now also implement MatchableHandlerMapping.getPatternParser() to expose the PathPatternParser we use. This is needed to detect that DelegatingHandlerMapping uses a PathPatternParser from the outside as the Spring MVC request processing pipeline treats those HandlerMappings in a special way. This is especially important in the context of spring-projects/spring-framework#26814 as that makes the HM implementations work properly in CORS preflight requests.

When used with Spring Security in place, CORS preflight requests are inspected through the HandlerMappingIntrospector, that looks up the CORS configuration for the handler to serve the actual request. That lookup previously failed as Spring MVC was unable to detect that Spring Data REST's DelegatingHandlerMapping works with a PathPatternParser and didn't properly set up the processing pipeline to eventually end up in an invalid state, produce an exception that caused the handler method lookup to fail and cause Spring Security to fail with a 403, claiming the request was invalid.

Fixes #2007.
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Aug 19, 2021
…correctly.

We now also implement MatchableHandlerMapping.getPatternParser() to expose the PathPatternParser we use. This is needed to detect that DelegatingHandlerMapping uses a PathPatternParser from the outside as the Spring MVC request processing pipeline treats those HandlerMappings in a special way. This is especially important in the context of spring-projects/spring-framework#26814 as that makes the HM implementations work properly in CORS preflight requests.

When used with Spring Security in place, CORS preflight requests are inspected through the HandlerMappingIntrospector, that looks up the CORS configuration for the handler to serve the actual request. That lookup previously failed as Spring MVC was unable to detect that Spring Data REST's DelegatingHandlerMapping works with a PathPatternParser and didn't properly set up the processing pipeline to eventually end up in an invalid state, produce an exception that caused the handler method lookup to fail and cause Spring Security to fail with a 403, claiming the request was invalid.

Fixes #2007, #2054.
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants