From 2e6046a65549d9523a35fe416c52c4f7bd9038e4 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 13 Dec 2024 14:51:22 +0000 Subject: [PATCH] Remove trailing slash matching in PathPatternParser See gh-34036 --- .../pattern/CaptureVariablePathElement.java | 5 - .../web/util/pattern/LiteralPathElement.java | 9 +- .../web/util/pattern/PathPattern.java | 18 +- .../web/util/pattern/PathPatternParser.java | 37 +-- .../web/util/pattern/RegexPathElement.java | 5 - .../SingleCharWildcardedPathElement.java | 9 +- .../web/util/pattern/WildcardPathElement.java | 12 +- .../web/util/pattern/PathPatternTests.java | 215 ------------------ 8 files changed, 8 insertions(+), 302 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/CaptureVariablePathElement.java b/spring-web/src/main/java/org/springframework/web/util/pattern/CaptureVariablePathElement.java index 8acf0c7907f..d02dc686cb4 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/CaptureVariablePathElement.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/CaptureVariablePathElement.java @@ -100,11 +100,6 @@ class CaptureVariablePathElement extends PathElement { else { // Needs to be at least one character #SPR15264 match = (pathIndex == matchingContext.pathLength); - if (!match && matchingContext.isMatchOptionalTrailingSeparator()) { - match = //(nextPos > candidateIndex) && - (pathIndex + 1) == matchingContext.pathLength && - matchingContext.isSeparator(pathIndex); - } } } else { diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/LiteralPathElement.java b/spring-web/src/main/java/org/springframework/web/util/pattern/LiteralPathElement.java index 5b8837251e6..413b685f00d 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/LiteralPathElement.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/LiteralPathElement.java @@ -78,14 +78,7 @@ class LiteralPathElement extends PathElement { return true; } else { - if (pathIndex == matchingContext.pathLength) { - return true; - } - else { - return (matchingContext.isMatchOptionalTrailingSeparator() && - (pathIndex + 1) == matchingContext.pathLength && - matchingContext.isSeparator(pathIndex)); - } + return (pathIndex == matchingContext.pathLength); } } else { diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java index c87a538cbeb..abd3157b5c5 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java @@ -112,9 +112,6 @@ public class PathPattern implements Comparable { /** The options to use to parse a pattern. */ private final PathContainer.Options pathOptions; - /** If this pattern has no trailing slash, allow candidates to include one and still match successfully. */ - private final boolean matchOptionalTrailingSeparator; - /** Will this match candidates in a case-sensitive way? (case sensitivity at parse time). */ private final boolean caseSensitive; @@ -152,12 +149,10 @@ public class PathPattern implements Comparable { private boolean catchAll = false; - @SuppressWarnings("deprecation") PathPattern(String patternText, PathPatternParser parser, @Nullable PathElement head) { this.patternString = patternText; this.parser = parser; this.pathOptions = parser.getPathOptions(); - this.matchOptionalTrailingSeparator = parser.isMatchOptionalTrailingSeparator(); this.caseSensitive = parser.isCaseSensitive(); this.head = head; @@ -202,8 +197,7 @@ public class PathPattern implements Comparable { */ public boolean matches(PathContainer pathContainer) { if (this.head == null) { - return !hasLength(pathContainer) || - (this.matchOptionalTrailingSeparator && pathContainerIsJustSeparator(pathContainer)); + return !hasLength(pathContainer); } else if (!hasLength(pathContainer)) { if (this.head instanceof WildcardTheRestPathElement || this.head instanceof CaptureTheRestPathElement) { @@ -226,9 +220,7 @@ public class PathPattern implements Comparable { @Nullable public PathMatchInfo matchAndExtract(PathContainer pathContainer) { if (this.head == null) { - return (hasLength(pathContainer) && - !(this.matchOptionalTrailingSeparator && pathContainerIsJustSeparator(pathContainer)) ? - null : PathMatchInfo.EMPTY); + return (hasLength(pathContainer) && !pathContainerIsJustSeparator(pathContainer) ? null : PathMatchInfo.EMPTY); } else if (!hasLength(pathContainer)) { if (this.head instanceof WildcardTheRestPathElement || this.head instanceof CaptureTheRestPathElement) { @@ -647,7 +639,7 @@ public class PathPattern implements Comparable { * candidate currently being considered for a match but also some accumulators for * extracted variables. */ - class MatchingContext { + static class MatchingContext { final PathContainer candidate; @@ -681,10 +673,6 @@ public class PathPattern implements Comparable { this.determineRemainingPath = true; } - public boolean isMatchOptionalTrailingSeparator() { - return matchOptionalTrailingSeparator; - } - public void set(String key, String value, MultiValueMap parameters) { if (this.extractedUriVariables == null) { this.extractedUriVariables = new HashMap<>(); diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternParser.java b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternParser.java index 655d602f102..829dd599540 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternParser.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternParser.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -35,40 +35,11 @@ import org.springframework.util.StringUtils; */ public class PathPatternParser { - private boolean matchOptionalTrailingSeparator = false; - private boolean caseSensitive = true; private PathContainer.Options pathOptions = PathContainer.Options.HTTP_PATH; - /** - * Configure whether a {@link PathPattern} produced by this parser should - * automatically match request paths with a trailing slash. - *

If set to {@code true} a {@code PathPattern} without a trailing slash - * will also match request paths with a trailing slash. If set to - * {@code false} a {@code PathPattern} will only match request paths with - * a trailing slash. - *

The default was changed in 6.0 from {@code true} to {@code false} in - * order to support the deprecation of the property. - * @deprecated transparent support for trailing slashes is deprecated as of - * 6.0 in favor of configuring explicit redirects through a proxy, - * Servlet/web filter, or a controller. - */ - @Deprecated(since = "6.0") - public void setMatchOptionalTrailingSeparator(boolean matchOptionalTrailingSeparator) { - this.matchOptionalTrailingSeparator = matchOptionalTrailingSeparator; - } - - /** - * Whether optional trailing slashing match is enabled. - * @deprecated as of 6.0 together with {@link #setMatchOptionalTrailingSeparator(boolean)}. - */ - @Deprecated(since = "6.0") - public boolean isMatchOptionalTrailingSeparator() { - return this.matchOptionalTrailingSeparator; - } - /** * Configure whether path pattern matching should be case-sensitive. *

The default is {@code true}. @@ -141,12 +112,6 @@ public class PathPatternParser { */ public static final PathPatternParser defaultInstance = new PathPatternParser() { - @SuppressWarnings("deprecation") - @Override - public void setMatchOptionalTrailingSeparator(boolean matchOptionalTrailingSeparator) { - raiseError(); - } - @Override public void setCaseSensitive(boolean caseSensitive) { raiseError(); diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/RegexPathElement.java b/spring-web/src/main/java/org/springframework/web/util/pattern/RegexPathElement.java index dff0609ac44..9dcf31549ed 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/RegexPathElement.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/RegexPathElement.java @@ -143,11 +143,6 @@ class RegexPathElement extends PathElement { // If pattern is capturing variables there must be some actual data to bind to them matches = (pathIndex + 1 >= matchingContext.pathLength) && (this.variableNames.isEmpty() || !textToMatch.isEmpty()); - if (!matches && matchingContext.isMatchOptionalTrailingSeparator()) { - matches = (this.variableNames.isEmpty() || !textToMatch.isEmpty()) && - (pathIndex + 2 >= matchingContext.pathLength) && - matchingContext.isSeparator(pathIndex + 1); - } } } else { diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/SingleCharWildcardedPathElement.java b/spring-web/src/main/java/org/springframework/web/util/pattern/SingleCharWildcardedPathElement.java index 3d83fc1df00..6c6223d0d52 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/SingleCharWildcardedPathElement.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/SingleCharWildcardedPathElement.java @@ -99,14 +99,7 @@ class SingleCharWildcardedPathElement extends PathElement { return true; } else { - if (pathIndex == matchingContext.pathLength) { - return true; - } - else { - return (matchingContext.isMatchOptionalTrailingSeparator() && - (pathIndex + 1) == matchingContext.pathLength && - matchingContext.isSeparator(pathIndex)); - } + return (pathIndex == matchingContext.pathLength); } } else { diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/WildcardPathElement.java b/spring-web/src/main/java/org/springframework/web/util/pattern/WildcardPathElement.java index 61c584f9c49..531a9bcc6ae 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/WildcardPathElement.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/WildcardPathElement.java @@ -60,16 +60,8 @@ class WildcardPathElement extends PathElement { return true; } else { - if (pathIndex == matchingContext.pathLength) { - // and the path data has run out too - return true; - } - else { - return (matchingContext.isMatchOptionalTrailingSeparator() && // if optional slash is on... - segmentData != null && !segmentData.isEmpty() && // and there is at least one character to match the *... - (pathIndex + 1) == matchingContext.pathLength && // and the next path element is the end of the candidate... - matchingContext.isSeparator(pathIndex)); // and the final element is a separator - } + // and the path data has run out too + return (pathIndex == matchingContext.pathLength); } } else { diff --git a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java index cdf9a475d97..6e03b645262 100644 --- a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java +++ b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java @@ -80,7 +80,6 @@ class PathPatternTests { @Test void basicMatching() { checkMatches("", ""); - checkMatches("", "/"); checkMatches("", null); checkNoMatch("/abc", "/"); checkMatches("/", "/"); @@ -99,211 +98,6 @@ class PathPatternTests { assertThat(pp.matches(toPathContainer(path))).isFalse(); } - @SuppressWarnings("deprecation") - @Test - void optionalTrailingSeparators() { - PathPattern pp; - // LiteralPathElement - pp = parse("/resource"); - assertMatches(pp,"/resource"); - assertMatches(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parse("/resource/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - - pp = parse("res?urce"); - assertNoMatch(pp,"resource//"); - // SingleCharWildcardPathElement - pp = parse("/res?urce"); - assertMatches(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parse("/res?urce/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - - // CaptureVariablePathElement - pp = parse("/{var}"); - assertMatches(pp,"/resource"); - assertThat(pp.matchAndExtract(toPathContainer("/resource")).getUriVariables()).containsEntry("var", "resource"); - assertMatches(pp,"/resource/"); - assertThat(pp.matchAndExtract(toPathContainer("/resource/")).getUriVariables()).containsEntry( - "var", - "resource" - ); - assertNoMatch(pp,"/resource//"); - pp = parse("/{var}/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertThat(pp.matchAndExtract(toPathContainer("/resource/")).getUriVariables()).containsEntry( - "var", - "resource" - ); - assertNoMatch(pp,"/resource//"); - - // CaptureTheRestPathElement - pp = parse("/{*var}"); - assertMatches(pp,"/resource"); - assertThat(pp.matchAndExtract(toPathContainer("/resource")).getUriVariables()).containsEntry( - "var", - "/resource" - ); - assertMatches(pp,"/resource/"); - assertThat(pp.matchAndExtract(toPathContainer("/resource/")).getUriVariables()).containsEntry( - "var", - "/resource/" - ); - assertMatches(pp,"/resource//"); - assertThat(pp.matchAndExtract(toPathContainer("/resource//")).getUriVariables()).containsEntry( - "var", - "/resource//" - ); - assertMatches(pp,"//resource//"); - assertThat(pp.matchAndExtract(toPathContainer("//resource//")).getUriVariables()).containsEntry( - "var", - "//resource//" - ); - - // WildcardTheRestPathElement - pp = parse("/**"); - assertMatches(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertMatches(pp,"/resource//"); - assertMatches(pp,"//resource//"); - - // WildcardPathElement - pp = parse("/*"); - assertMatches(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parse("/*/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - - // RegexPathElement - pp = parse("/{var1}_{var2}"); - assertMatches(pp,"/res1_res2"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2")).getUriVariables()).containsEntry("var1", "res1"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2")).getUriVariables()).containsEntry("var2", "res2"); - assertMatches(pp,"/res1_res2/"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2/")).getUriVariables()).containsEntry("var1", "res1"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2/")).getUriVariables()).containsEntry("var2", "res2"); - assertNoMatch(pp,"/res1_res2//"); - pp = parse("/{var1}_{var2}/"); - assertNoMatch(pp,"/res1_res2"); - assertMatches(pp,"/res1_res2/"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2/")).getUriVariables()).containsEntry("var1", "res1"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2/")).getUriVariables()).containsEntry("var2", "res2"); - assertNoMatch(pp,"/res1_res2//"); - pp = parse("/{var1}*"); - assertMatches(pp,"/a"); - assertMatches(pp,"/a/"); - assertNoMatch(pp,"/"); // no characters for var1 - assertNoMatch(pp,"//"); // no characters for var1 - - // Now with trailing matching turned OFF - PathPatternParser parser = new PathPatternParser(); - parser.setMatchOptionalTrailingSeparator(false); - // LiteralPathElement - pp = parser.parse("/resource"); - assertMatches(pp,"/resource"); - assertNoMatch(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parser.parse("/resource/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - - // SingleCharWildcardPathElement - pp = parser.parse("/res?urce"); - assertMatches(pp,"/resource"); - assertNoMatch(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parser.parse("/res?urce/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - - // CaptureVariablePathElement - pp = parser.parse("/{var}"); - assertMatches(pp,"/resource"); - assertThat(pp.matchAndExtract(toPathContainer("/resource")).getUriVariables()).containsEntry("var", "resource"); - assertNoMatch(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parser.parse("/{var}/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertThat(pp.matchAndExtract(toPathContainer("/resource/")).getUriVariables()).containsEntry( - "var", - "resource" - ); - assertNoMatch(pp,"/resource//"); - - // CaptureTheRestPathElement - pp = parser.parse("/{*var}"); - assertMatches(pp,"/resource"); - assertThat(pp.matchAndExtract(toPathContainer("/resource")).getUriVariables()).containsEntry( - "var", - "/resource" - ); - assertMatches(pp,"/resource/"); - assertThat(pp.matchAndExtract(toPathContainer("/resource/")).getUriVariables()).containsEntry( - "var", - "/resource/" - ); - assertMatches(pp,"/resource//"); - assertThat(pp.matchAndExtract(toPathContainer("/resource//")).getUriVariables()).containsEntry( - "var", - "/resource//" - ); - assertMatches(pp,"//resource//"); - assertThat(pp.matchAndExtract(toPathContainer("//resource//")).getUriVariables()).containsEntry( - "var", - "//resource//" - ); - - // WildcardTheRestPathElement - pp = parser.parse("/**"); - assertMatches(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertMatches(pp,"/resource//"); - assertMatches(pp,"//resource//"); - - // WildcardPathElement - pp = parser.parse("/*"); - assertMatches(pp,"/resource"); - assertNoMatch(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - pp = parser.parse("/*/"); - assertNoMatch(pp,"/resource"); - assertMatches(pp,"/resource/"); - assertNoMatch(pp,"/resource//"); - - // RegexPathElement - pp = parser.parse("/{var1}_{var2}"); - assertMatches(pp,"/res1_res2"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2")).getUriVariables()).containsEntry("var1", "res1"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2")).getUriVariables()).containsEntry("var2", "res2"); - assertNoMatch(pp,"/res1_res2/"); - assertNoMatch(pp,"/res1_res2//"); - pp = parser.parse("/{var1}_{var2}/"); - assertNoMatch(pp,"/res1_res2"); - assertMatches(pp,"/res1_res2/"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2/")).getUriVariables()).containsEntry("var1", "res1"); - assertThat(pp.matchAndExtract(toPathContainer("/res1_res2/")).getUriVariables()).containsEntry("var2", "res2"); - assertNoMatch(pp,"/res1_res2//"); - pp = parser.parse("/{var1}*"); - assertMatches(pp,"/a"); - assertNoMatch(pp,"/a/"); - assertNoMatch(pp,"/"); // no characters for var1 - assertNoMatch(pp,"//"); // no characters for var1 - } - @Test void pathRemainderBasicCases_spr15336() { // Cover all PathElement kinds @@ -493,12 +287,9 @@ class PathPatternTests { checkMatches("*a*", "testa"); checkMatches("a/*", "a/"); checkNoMatch("a/*", "a//"); // no data for * - checkMatches("a/*", "a/a/"); // trailing slash, so is allowed PathPatternParser ppp = new PathPatternParser(); - ppp.setMatchOptionalTrailingSeparator(false); assertThat(ppp.parse("a/*").matches(toPathContainer("a//"))).isFalse(); checkMatches("a/*", "a/a"); - checkMatches("a/*", "a/a/"); // trailing slash is optional checkMatches("/resource/**", "/resource"); checkNoMatch("/resource/**", "/resourceX"); checkNoMatch("/resource/**", "/resourceX/foobar"); @@ -554,7 +345,6 @@ class PathPatternTests { checkMatches("test*aaa", "testblaaaa"); checkNoMatch("test*", "tst"); checkNoMatch("test*", "tsttest"); - checkMatches("test*", "test/"); // trailing slash is optional checkMatches("test*", "test"); // trailing slash is optional checkNoMatch("test*", "test/t"); checkNoMatch("test/*", "test"); @@ -805,7 +595,6 @@ class PathPatternTests { // Only patterns not capturing variables cannot match against just / PathPatternParser ppp = new PathPatternParser(); - ppp.setMatchOptionalTrailingSeparator(true); pp = ppp.parse("/****"); assertMatches(pp,"/abcdef"); assertMatches(pp,"/"); @@ -1185,10 +974,8 @@ class PathPatternTests { return parse(pattern).matchAndExtract(PathPatternTests.toPathContainer(path)); } - @SuppressWarnings("deprecation") private PathPattern parse(String path) { PathPatternParser pp = new PathPatternParser(); - pp.setMatchOptionalTrailingSeparator(true); return pp.parse(path); } @@ -1199,10 +986,8 @@ class PathPatternTests { return PathContainer.parsePath(path); } - @SuppressWarnings("deprecation") private void checkMatches(String uriTemplate, String path) { PathPatternParser parser = new PathPatternParser(); - parser.setMatchOptionalTrailingSeparator(true); PathPattern p = parser.parse(uriTemplate); PathContainer pc = toPathContainer(path); assertThat(p.matches(pc)).isTrue();