MappedInterceptor in 5.3 does not support all AntPatternMatcher patterns

[protobufel2]

Spring Framework 5.3 (up to 5.3.5) MappedInterceptor's constructor effectively prevents the use of any PathMatcher, including legacy AntPathMatcher, making it completely incompatible with all earlier versions. Here is why:

1. The InterceptorRegistration#getInterceptor tries to create MappedInterceptor, however it'll fail, due to 2.

	protected Object getInterceptor() {

		if (this.includePatterns == null && this.excludePatterns == null) {
			return this.interceptor;
		}

		MappedInterceptor mappedInterceptor = new MappedInterceptor(
				StringUtils.toStringArray(this.includePatterns),
				StringUtils.toStringArray(this.excludePatterns),
				this.interceptor);

		if (this.pathMatcher != null) {
			mappedInterceptor.setPathMatcher(this.pathMatcher);
		}

		return mappedInterceptor;
	}

2. MappedInterceptor's base constructor calls MappedInterceptor#initPatterns, which would use the default PathPatternParser and will fail for the Ant-like patterns and suffixes (old style) no matter whether set in PathMatchConfigurer or not:

	public MappedInterceptor(@Nullable String[] includePatterns, @Nullable String[] excludePatterns,
			HandlerInterceptor interceptor, @Nullable PathPatternParser parser) {

		this.includePatterns = initPatterns(includePatterns, parser);
		this.excludePatterns = initPatterns(excludePatterns, parser);
		this.interceptor = interceptor;
	}

	@Nullable
	private static PathPattern[] initPatterns(
			@Nullable String[] patterns, @Nullable PathPatternParser parser) {

		if (ObjectUtils.isEmpty(patterns)) {
			return null;
		}
		parser = (parser != null ? parser : PathPatternParser.defaultInstance);
		return Arrays.stream(patterns).map(parser::parse).toArray(PathPattern[]::new);
	}

The problem is that include/excludePatterns type have changed from String[] to PathPattern[]. As a possible solution to this could be having a chameleon PathPattern, i.e. NOOP PathPattern holding just the String underneath and having isNoop() to differentiate from the real ones, to be used in concert with the MappedInterceptor constructor with a PathMatcher parameter, and the rest of the MappedInterceptor code checking for isNoop() accordingly.
In addition, MappedInterceptor(@Nullable String[] includePatterns, @Nullable String[] excludePatterns, HandlerInterceptor interceptor) can be using such NOOP PathPattern and be compatible with InterceptorRegistration#getInterceptor and friends.

Unless there is a simple and obvious solution/config already, this is a major, showstopper bug.

Thanks much in advance,
David

[rstoyanchev]

MappedInterceptor was changed as part of a broader set of changes to support use of a parsed PathPattern as an alternative to String pattern matching via PathMatcher. For a HandlerMapping which one to use is based on whether a PathPatternParser is set but a MappedInterceptor has to be able to adapt to a degree depending on which HandlerMapping is it intercepting. This is why it helps to keep patterns parsed.

The idea is that the syntax is largely the same and it shouldn't matter, and especially a mapped interceptor isn't going to run into the number and variety of patterns that one might see on annotated controllers. That said there are indeed some differences which make this a regression for your application.

Keep in mind that mapping by extension has been deprecated in 5.3 as explained in this section of the reference and there are an inherent security threat.

Before we discuss how to address, can you confirm the exact types of pattern issues you ran into? Is it something like "/foo" that should also support "/foo.*" or is there more? One obvious option would be to explicitly add the extension patterns.

[protobufel2]

Any suffixes like /mypath/*/*.abc and any ** in the middle like /mypath/**/xyz, all typical AntPathMatcher paths not supported by PathPattern and PathPatternParser.

I agree with suffixes being a security issue and the apparent ambiguity of ‘**’ in the middle patterns, however, the MappedInterceptor constructor makes these impossible, whereas the documentation and the code elsewhere, including in the class itself, assumes the compatibility with AntPathMatcher is still there, hence the problem. In reality, Spring 5.3 doesn’t just deprecate but eliminates the use of AntPathMatcher and that is a big regression, hence the proposed fix.

[rstoyanchev]

Any suffixes like /mypath//.abc and any ** in the middle like /mypath/**/xyz, all typical AntPathMatcher paths not supported by PathPattern and PathPatternParser.

That's what I'm trying to narrow down. The first /mypath/*/*.abc is supported. What is not supported is ** in the middle because very often it leads to ambiguous and not intuitive mappings. There are countless issues related to that. That is a relatively minor and less common difference.

[rstoyanchev]

I'm trying to establish which patterns you have. If it is about suffix pattern matching, then this is not related to the changes in MappedInteceptor. It has to do with the fact that we disabled suffix pattern matching by default but there is a way to re-enable it.

[protobufel2]

I've used the documented PathMatchConfigurer specifically for the old behavior, see below, to no avail. As shown above, it will
eventually use MappedInterceptor constructor and throws the error about ** in the middle and also doesn't allow suffixes.
Here is the code:

@Configuration
@EnableWebMvc
public class WebConfig implements WebMvcConfigurer {

  @Override
  public void configurePathMatch(final PathMatchConfigurer configurer) {
    configurer
        .setPathMatcher(new AntPathMatcher())
        .setUseSuffixPatternMatch(true)
        .setUseRegisteredSuffixPatternMatch(true)
        //        .setPatternParser(null)
        .setUrlPathHelper(UrlPathHelper.defaultInstance);
  }

  @Override
  public void configureContentNegotiation(final ContentNegotiationConfigurer configurer) {
    configurer.favorPathExtension(true);
  }

  @Override
  public void addInterceptors(final InterceptorRegistry registry) {
     registry
        .addInterceptor(new MyInterceptor())
        .addPathPatterns("/**")
        .excludePathPatterns("/", "/**/*.css", "/path1/**/path2");
  }
}

[rstoyanchev]

This should be fixed now. If you can please give it a try with 5.3.6 snapshots.

[protobufel2]

Thank you so much, @rstoyanchev, it works!

[kjogi]

@rstoyanchev this issue is happening with spring 6.0.8. Did something change again? I can see that this constructor is getting called -
org.springframework.web.servlet.handler.MappedInterceptor#MappedInterceptor(java.lang.String[], java.lang.String[], org.springframework.web.servlet.HandlerInterceptor)

And the parser being passed is always null, so PathPatternParser is used. And if the pattern has a/**/b, then the parsing fails with error No more pattern data allowed after '{*...}' or '**' pattern element essentially failing for Ant style paths.

I am already overriding configurePathMatch as mentioned here #26690 (comment).

I am not sure if something has changed by default in 6.0.8, but if you can suggest how to fix this, it would be great. This is a major blocker for moving to Spring 6.

[rstoyanchev]

Nothing significant has changed in MappedInterceptor. The parse error should be caught and ignored, leaving the PatternAdapter with only a String pattern, so at runtime it should use the String pattern with AntPathMatcher. You can debug these lines to check. If necessary, please create a new issue and provide a small sample that we can debug.