Affects: 5.3.13

After upgrading an application to Spring Boot 2.6.x we noticed a problem when using the path_pattern_parser which is configured by default (instead of the "legacy" ant_path_matcher).

Not sure if it's a bug or a bug fix .... but it's a different behavior and it wasn't so obvious to me at the origin that the Path Matching Strategy change was the root cause ( thx @bclozel )

Let's say you have a controller with these 2 methods:

    @GetMapping(value = "/api/test/{fileId:^(?!%25).*}")
    public ResponseEntity<String> shouldNotMatch(@PathVariable String fileId) {
        return ResponseEntity.badRequest().build();
    }

    @GetMapping(value = "/api/test/{ticketId:[0-9]+}%2F{filename}")
    public ResponseEntity<String> shouldMatch(@PathVariable String ticketId, @PathVariable String filename) {
        return ResponseEntity.ok().build();
    }

If the path called is /api/test/12345%252Ffoo.zip then using the ant_path_matcher the dispatcher goes to shouldMatch. But if using path_pattern_parser it goes to shouldNotMatch

To demo the issue I created a project : https://github.com/aheritier/path_pattern_parser

The controller : https://github.com/aheritier/path_pattern_parser/blob/main/src/main/java/com/example/demo/AController.java

The test: https://github.com/aheritier/path_pattern_parser/blob/main/src/test/java/com/example/demo/AControllerTestBase.java

It's ok with ant_path_matcher: https://github.com/aheritier/path_pattern_parser/blob/main/src/test/java/com/example/demo/AControllerWithAntPathMatcherTest.java

It's ko with path_pattern_parser: https://github.com/aheritier/path_pattern_parser/blob/main/src/test/java/com/example/demo/AControllerWithPathPatternParserTest.java

Comment From: bclozel

Thanks for the report and the sample @aheritier !

I've tracked this down to a behavior change between AntPathMatcher and PathPatternParser - not in the matching algorithm (because both pattern match the given request), but in the ordering phase. Here's a simplified test showing the difference:

@Test
void gh27759() {
    AntPathMatcher antPathMatcher = new AntPathMatcher();
    Comparator<String> patternComparator = antPathMatcher.getPatternComparator("/api/test/12345%252Ffoo.zip");
    int result = patternComparator.compare("/api/test/{ticketId:[0-9]+}%2F{filename}", "/api/test/{fileId:^(?!%25).*}");
    assertThat(result).isNegative();

    PathPatternParser parser = new PathPatternParser();
    PathPattern shouldMatchPathPattern = parser.parse("/api/test/{ticketId:[0-9]+}%2F{filename}");
    PathPattern shouldNotMatchPathPattern = parser.parse("/api/test/{fileId:^(?!%25).*}");
    assertThat(shouldMatchPathPattern.compareTo(shouldNotMatchPathPattern)).isPositive();
}

I'm not sure we can call that a regression, since AntPathMatcher has shown over the years many under-specified behavior. PathPatternParser is way more consistent and implements a scoring system for consistent pattern ordering. In this case, I believe "/api/test/{fileId:^(?!%25).*}" is considered as more specific because of its score (only one dynamic part in the segment vs. two).

While testing various things, I noticed that both patterns actually match here - is that the expected behavior you were looking for? Or maybe you were aiming for a different negative lookahead with your pattern, like "/api/test/{fileId:^((?!%25).)*$}"?

Note that with PathPatternParser, you can also use {*path} to match the remaining part of the path and potentially apply custom matching/validation in the controller handler directly to get more control over the matching/sorting phase.

Comment From: aheritier

Thanks for looking at it @bclozel I didn't write the original code and like you when I tried to create the testcase and to have something very simple I found that the patterns were strange and potentially buggy. That's why I preferred to report it as a behavior change and not necessarily a bug.

Both methods should be exclusive. Either the path contains a / (= %2F = %252F) and in such case it should go only to shouldMatch or if it doesn't contain a / it should go to shouldNotMatch. The developper intent (AFIU) was to use a negative lookahead for such purpose and I agree with you "/api/test/{fileId:^(?!%25).*}" is wrong from my POV and we should use "/api/test/{fileId:^((?!%25).)*$}" but when I tried it I got an error about the number of capturing groups in the regexp.

org.springframework.web.util.NestedServletException: Request processing failed; nested exception is java.lang.IllegalArgumentException: No capture groups allowed in the constraint regex: ^((?!%25).)*$
Caused by: java.lang.IllegalArgumentException: No capture groups allowed in the constraint regex: ^((?!%25).)*$

I'll switch to the PathPatternParser with pleasure if I cannot easily define a rule to match exclusively number/something and anything_with_no_slash I agree that it should be easier to have only one method and manage the logic within it

Comment From: bclozel

My bad, my previous sample had a capturing group and we needed a non-capturing one. To summarize, "/api/test/{fileId:^(?:(?!%2F).)*$}" should work.

As you've seen, we're really betting a lot on regexp and how URL encoding/decoding is handled during matching. I think a pattern like "/api/test/{*filePath}" would be much more consistent and you could deal with the specific logic where it should be.

I've updated my sample test to show the behavior here:

PathContainer pathContainingSlash = PathContainer.parsePath("/api/test/12345%252Ffoo.zip");
PathContainer pathWithoutSlash = PathContainer.parsePath("/api/test/12345%2Ffoo.zip");
PathPatternParser parser = new PathPatternParser();

PathPattern shouldMatchSlash = parser.parse("/api/test/{ticketId:[0-9]+}%2F{filename}");
assertThat(shouldMatchSlash.matches(pathContainingSlash)).isTrue();
assertThat(shouldMatchSlash.matches(pathWithoutSlash)).isFalse();

PathPattern shouldNotMatchSlash = parser.parse("/api/test/{fileId:^(?:(?!%2F).)*$}");
assertThat(shouldNotMatchSlash.matches(pathContainingSlash)).isFalse();
assertThat(shouldNotMatchSlash.matches(pathWithoutSlash)).isTrue();

PathPattern filePattern = parser.parse("/api/test/{*filePath}");
PathPattern.PathMatchInfo withSlash = filePattern.matchAndExtract(pathContainingSlash);
assertThat(withSlash.getUriVariables()).containsEntry("filePath", "/12345%2Ffoo.zip");
PathPattern.PathMatchInfo withoutSlash = filePattern.matchAndExtract(pathWithoutSlash);
assertThat(withoutSlash.getUriVariables()).containsEntry("filePath", "/12345/foo.zip");

I'm closing this issue for the following reasons: * I don't think we can improve the comparison/sorting algorithm here, since it's well more defined than AntPathMatcher * In this case, we might be building too much logic in the pattern matching and using the new "capture the rest" feature is a good solution.

AntPathMatcher is not scheduled to go away, so feel free to override the configuration property and maybe later refactor this part of the codebase. The PathPatternParser infrastructure has been introduced with WebFlux and progressively rolled in MVC, it's always nice to check the behavior changes. Thanks for the report!

Comment From: aheritier

It's ok for me @bclozel You gave me different solutions/workarounds and I agree that making more complex the comparison/sorting algorithm doesn't makes sense for such exotic/rare use-case. At least it's now documented if someone is facing a similar issue they should find some options to update their project Thanks for your help