spring-framework icon indicating copy to clipboard operation
spring-framework copied to clipboard

AntPathMatcher has an unintuitive way of matching "**"

Open hannes-angst opened this issue 6 years ago • 1 comments

Summary

Using security.basic.path in spring properties may lead to unexpected behavior because of the current way, AntPathRequestMatcher interprets the defined paths.

Actual Behavior

When defining security.basic.path=/abc** sub-paths like/abc/d or /abc/ are not included.

Expected Behavior

When defining security.basic.path=/abc** any request matching this prefix (/abc) should be included regardless of following slashes.

Configuration

spring.properties:

 #
 # security Configuration
 #
 security.basic.enabled=true
 security.enable-csrf=false
 security.sessions=stateless
 security.basic.path=/abc**

Version

Spring-Boot 1.5.13.RELEASE Spring Security 4.2.6.RELEASE

(I am fully aware, that /abc/** includes /abc but that's not the point here)

hannes-angst avatar Jul 04 '18 06:07 hannes-angst

Thank you for the report, but AntPathRequestMatcher is implemented using AntPathMatcher which is a Framework bit of code. Any changes would need to happen in Spring Framework.

rwinch avatar Sep 07 '22 20:09 rwinch

		assertThat(pathMatcher.match("/abc/**", "/abc")).isTrue();
		assertThat(pathMatcher.match("/abc/**", "/abc/")).isTrue();
		assertThat(pathMatcher.match("/abc/**", "/abc/d")).isTrue();
		assertThat(pathMatcher.match("/abc/**", "/abc/d/e")).isTrue();
		assertThat(pathMatcher.match("/abc/**", "/abc/d/e/")).isTrue();

		assertThat(pathMatcher.match("/abc**", "/abc")).isTrue();
		assertThat(pathMatcher.match("/abc**", "/abc/")).isFalse()
		assertThat(pathMatcher.match("/abc**", "/abc/d")).isFalse();;
		assertThat(pathMatcher.match("/abc**", "/abc/d/e")).isFalse();
		assertThat(pathMatcher.match("/abc**", "/abc/d/e/")).isFalse();

To match all sub directories in AntStylePatternMatch, I think we should use /abc/**, not /abc**. It's actually AntStyle spec.

You think should we also allow /abc** to match all sub directories of /abc?

injae-kim avatar Jan 02 '24 14:01 injae-kim

Thanks for follow-up @injae-kim. I agree that we should not expand the behavior here and make "/abc**" match "/abc/d". I think there is a fundamental difference between "*" use cases:

  • "*" as a pattern segment like "/a/*/c" matches a single segment like "/a/test/c" or "/a/other/c"
  • "*" in a pattern segment like "/a/t*/c" matches a single segment like "/a/test/c" but not "/a/other/c"
  • "**" can only be used as a pattern segment like "/a/**" as changing this would raise questions like the expected behavior of "/a/t**t/c" - should it match "/a/test/c" and "/a/t/other/t/c" ?

Our implementation sticks to the principles of https://ant.apache.org/manual/dirtasks.html#patterns and I don't think we should change that now. In the meantime, for all web-related patterns we introduced PathPattern which has better performance and is much more consistent.

I'm declining this enhancement request as a result.

bclozel avatar Jan 02 '24 14:01 bclozel