Reject "/path/**/other" patterns in PathPatternParser

Prior to this commit, patterns like `"/path/**/other"` would be treated
as `"/path/*/other"` (single wildcard, i.e. matching zero to many chars
within a path segment). This will not match multiple segments, as
expected by `AntPathMatcher` users or by `PathPatternParser` users when
in patterns like `"/resource/**"`.

This commit now rejects patterns like `"/path/**/other"` as invalid.
This behavior was previously warned against since gh-24958.

Closes gh-24952

Author: bclozel

spring-web/src/main/java/org/springframework/web/util/pattern/InternalPathPatternParser.java

@@ -236,7 +236,7 @@ else if (ch == '}' && !previousBackslash) {
 
 	/**
 	 * After processing a separator, a quick peek whether it is followed by
-	 * (and only before the end of the pattern or the next separator).
+	 * a double wildcard (and only as the last path element).
 	 */
 	private boolean peekDoubleWildcard() {
 		if ((this.pos + 2) >= this.pathPatternLength) {
@@ -245,6 +245,11 @@ private boolean peekDoubleWildcard() {
 		if (this.pathPatternData[this.pos + 1] != '*' || this.pathPatternData[this.pos + 2] != '*') {
 			return false;
 		}
+		char separator = this.parser.getPathOptions().separator();
+		if ((this.pos + 3) < this.pathPatternLength && this.pathPatternData[this.pos + 3] == separator) {
+			throw new PatternParseException(this.pos, this.pathPatternData,
+					PatternMessage.NO_MORE_DATA_EXPECTED_AFTER_CAPTURE_THE_REST);
+		}
 		return (this.pos + 3 == this.pathPatternLength);
 	}
 

spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternParser.java

@@ -16,9 +16,6 @@
 
 package org.springframework.web.util.pattern;
 
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
-
 import org.springframework.http.server.PathContainer;
 
 /**
@@ -37,8 +34,6 @@
  */
 public class PathPatternParser {
 
-	private static final Log logger = LogFactory.getLog(PathPatternParser.class);
-
 	private boolean matchOptionalTrailingSeparator = true;
 
 	private boolean caseSensitive = true;
@@ -114,11 +109,6 @@ public PathContainer.Options getPathOptions() {
 	 * @throws PatternParseException in case of parse errors
 	 */
 	public PathPattern parse(String pathPattern) throws PatternParseException {
-		int wildcardIndex = pathPattern.indexOf("**" + this.pathOptions.separator());
-		if (wildcardIndex != -1 && wildcardIndex != pathPattern.length() - 3) {
-			logger.warn("'**' patterns are not supported in the middle of patterns and will be rejected in the future. " +
-					"Consider using '*' instead for matching a single path segment.");
-		}
 		return new InternalPathPatternParser(this).parse(pathPattern);
 	}
 

spring-web/src/main/java/org/springframework/web/util/pattern/PatternParseException.java

@@ -100,7 +100,7 @@ public enum PatternMessage {
 		CANNOT_HAVE_ADJACENT_CAPTURES("Adjacent captures are not allowed"),
 		ILLEGAL_CHARACTER_AT_START_OF_CAPTURE_DESCRIPTOR("Char ''{0}'' not allowed at start of captured variable name"),
 		ILLEGAL_CHARACTER_IN_CAPTURE_DESCRIPTOR("Char ''{0}'' is not allowed in a captured variable name"),
-		NO_MORE_DATA_EXPECTED_AFTER_CAPTURE_THE_REST("No more pattern data allowed after '{*...}' pattern element"),
+		NO_MORE_DATA_EXPECTED_AFTER_CAPTURE_THE_REST("No more pattern data allowed after '{*...}' or '**' pattern element"),
 		BADLY_FORMED_CAPTURE_THE_REST("Expected form when capturing the rest of the path is simply '{*...}'"),
 		MISSING_REGEX_CONSTRAINT("Missing regex constraint on capture"),
 		ILLEGAL_DOUBLE_CAPTURE("Not allowed to capture ''{0}'' twice in the same pattern"),

spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternParserTests.java

@@ -28,6 +28,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.fail;
 
 /**
@@ -128,12 +129,12 @@ public void regexPathElementPatterns() {
 		pathPattern = checkStructure("/{var:\\\\}");
 		PathElement next = pathPattern.getHeadSection().next;
 		assertThat(next.getClass().getName()).isEqualTo(CaptureVariablePathElement.class.getName());
-		assertMatches(pathPattern,"/\\");
+		assertMatches(pathPattern, "/\\");
 
 		pathPattern = checkStructure("/{var:\\/}");
 		next = pathPattern.getHeadSection().next;
 		assertThat(next.getClass().getName()).isEqualTo(CaptureVariablePathElement.class.getName());
-		assertNoMatch(pathPattern,"/aaa");
+		assertNoMatch(pathPattern, "/aaa");
 
 		pathPattern = checkStructure("/{var:a{1,2}}");
 		next = pathPattern.getHeadSection().next;
@@ -142,25 +143,25 @@ public void regexPathElementPatterns() {
 		pathPattern = checkStructure("/{var:[^\\/]*}");
 		next = pathPattern.getHeadSection().next;
 		assertThat(next.getClass().getName()).isEqualTo(CaptureVariablePathElement.class.getName());
-		PathPattern.PathMatchInfo result = matchAndExtract(pathPattern,"/foo");
+		PathPattern.PathMatchInfo result = matchAndExtract(pathPattern, "/foo");
 		assertThat(result.getUriVariables().get("var")).isEqualTo("foo");
 
 		pathPattern = checkStructure("/{var:\\[*}");
 		next = pathPattern.getHeadSection().next;
 		assertThat(next.getClass().getName()).isEqualTo(CaptureVariablePathElement.class.getName());
-		result = matchAndExtract(pathPattern,"/[[[");
+		result = matchAndExtract(pathPattern, "/[[[");
 		assertThat(result.getUriVariables().get("var")).isEqualTo("[[[");
 
 		pathPattern = checkStructure("/{var:[\\{]*}");
 		next = pathPattern.getHeadSection().next;
 		assertThat(next.getClass().getName()).isEqualTo(CaptureVariablePathElement.class.getName());
-		result = matchAndExtract(pathPattern,"/{{{");
+		result = matchAndExtract(pathPattern, "/{{{");
 		assertThat(result.getUriVariables().get("var")).isEqualTo("{{{");
 
 		pathPattern = checkStructure("/{var:[\\}]*}");
 		next = pathPattern.getHeadSection().next;
 		assertThat(next.getClass().getName()).isEqualTo(CaptureVariablePathElement.class.getName());
-		result = matchAndExtract(pathPattern,"/}}}");
+		result = matchAndExtract(pathPattern, "/}}}");
 		assertThat(result.getUriVariables().get("var")).isEqualTo("}}}");
 
 		pathPattern = checkStructure("*");
@@ -249,10 +250,10 @@ public void illegalCapturePatterns() {
 		PathPattern pp = parse("/{abc:foo(bar)}");
 		assertThatIllegalArgumentException().isThrownBy(() ->
 				pp.matchAndExtract(toPSC("/foo")))
-			.withMessage("No capture groups allowed in the constraint regex: foo(bar)");
+				.withMessage("No capture groups allowed in the constraint regex: foo(bar)");
 		assertThatIllegalArgumentException().isThrownBy(() ->
 				pp.matchAndExtract(toPSC("/foobar")))
-			.withMessage("No capture groups allowed in the constraint regex: foo(bar)");
+				.withMessage("No capture groups allowed in the constraint regex: foo(bar)");
 	}
 
 	@Test
@@ -414,12 +415,12 @@ public void compareTests() {
 		assertThat(patterns.get(1)).isEqualTo(p2);
 	}
 
-	@Test // Should be updated with gh-24952
-	public void doubleWildcardWithinPatternNotSupported() {
+	@Test
+	public void captureTheRestWithinPatternNotSupported() {
 		PathPatternParser parser = new PathPatternParser();
-		PathPattern pattern = parser.parse("/resources/**/details");
-		assertThat(pattern.matches(PathContainer.parsePath("/resources/test/details"))).isTrue();
-		assertThat(pattern.matches(PathContainer.parsePath("/resources/projects/spring/details"))).isFalse();
+		assertThatThrownBy(() -> parser.parse("/resources/**/details"))
+				.isInstanceOf(PatternParseException.class)
+				.extracting("messageType").isEqualTo(PatternMessage.NO_MORE_DATA_EXPECTED_AFTER_CAPTURE_THE_REST);
 	}
 
 	@Test
@@ -461,16 +462,16 @@ private void checkError(String pattern, int expectedPos, PatternMessage expected
 			String... expectedInserts) {
 
 		assertThatExceptionOfType(PatternParseException.class)
-			.isThrownBy(() -> pathPattern = parse(pattern))
-			.satisfies(ex -> {
-				if (expectedPos >= 0) {
-					assertThat(ex.getPosition()).as(ex.toDetailedString()).isEqualTo(expectedPos);
-				}
-				assertThat(ex.getMessageType()).as(ex.toDetailedString()).isEqualTo(expectedMessage);
-				if (expectedInserts.length != 0) {
-					assertThat(ex.getInserts()).isEqualTo(expectedInserts);
-				}
-			});
+				.isThrownBy(() -> pathPattern = parse(pattern))
+				.satisfies(ex -> {
+					if (expectedPos >= 0) {
+						assertThat(ex.getPosition()).as(ex.toDetailedString()).isEqualTo(expectedPos);
+					}
+					assertThat(ex.getMessageType()).as(ex.toDetailedString()).isEqualTo(expectedMessage);
+					if (expectedInserts.length != 0) {
+						assertThat(ex.getInserts()).isEqualTo(expectedInserts);
+					}
+				});
 	}
 
 	@SafeVarargs

spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -54,8 +54,10 @@ public void pathContainer() {
 	public void hasPatternSyntax() {
 		PathPatternParser parser = new PathPatternParser();
 		assertThat(parser.parse("/foo/*").hasPatternSyntax()).isTrue();
-		assertThat(parser.parse("/foo/**/bar").hasPatternSyntax()).isTrue();
+		assertThat(parser.parse("/foo/**").hasPatternSyntax()).isFalse();
+		assertThat(parser.parse("/foo/{*elem}").hasPatternSyntax()).isFalse();
 		assertThat(parser.parse("/f?o").hasPatternSyntax()).isTrue();
+		assertThat(parser.parse("/f*").hasPatternSyntax()).isTrue();
 		assertThat(parser.parse("/foo/{bar}/baz").hasPatternSyntax()).isTrue();
 		assertThat(parser.parse("/foo/bar").hasPatternSyntax()).isFalse();
 	}
@@ -867,13 +869,10 @@ public void combine() {
 		assertThat(pathMatcher.combine("", "/hotels")).isEqualTo("/hotels");
 		assertThat(pathMatcher.combine("/hotels/*", "booking")).isEqualTo("/hotels/booking");
 		assertThat(pathMatcher.combine("/hotels/*", "/booking")).isEqualTo("/hotels/booking");
-		assertThat(pathMatcher.combine("/hotels/**", "booking")).isEqualTo("/hotels/**/booking");
-		assertThat(pathMatcher.combine("/hotels/**", "/booking")).isEqualTo("/hotels/**/booking");
 		assertThat(pathMatcher.combine("/hotels", "/booking")).isEqualTo("/hotels/booking");
 		assertThat(pathMatcher.combine("/hotels", "booking")).isEqualTo("/hotels/booking");
 		assertThat(pathMatcher.combine("/hotels/", "booking")).isEqualTo("/hotels/booking");
 		assertThat(pathMatcher.combine("/hotels/*", "{hotel}")).isEqualTo("/hotels/{hotel}");
-		assertThat(pathMatcher.combine("/hotels/**", "{hotel}")).isEqualTo("/hotels/**/{hotel}");
 		assertThat(pathMatcher.combine("/hotels", "{hotel}")).isEqualTo("/hotels/{hotel}");
 		assertThat(pathMatcher.combine("/hotels", "{hotel}.*")).isEqualTo("/hotels/{hotel}.*");
 		assertThat(pathMatcher.combine("/hotels/*/booking", "{booking}")).isEqualTo("/hotels/*/booking/{booking}");