Copilot commented on code in PR #2836:
URL: https://github.com/apache/shiro/pull/2836#discussion_r3621024134
##########
web/src/test/java/org/apache/shiro/web/filter/mgt/PathMatchingFilterChainResolverTest.java:
##########
@@ -290,4 +290,29 @@ void testGetChainWhenPathDoesNotEndWithSlash() {
assertThat(resolved).isNotNull();
verify(request).getServletPath();
}
+
+ /**
+ * Verifies that path traversal above root (where normalize returns null)
+ * no longer bypasses Shiro. The path is normalized to "/" and can match
+ * the /** catch-all chain if one exists.
+ */
+ @Test
+ void testPathTraversalAboveRootFallsBackToCatchAll() {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ FilterChain chain = mock(FilterChain.class);
+
+ // Create the /** catch-all chain (as ShiroFactoryBean would)
+ resolver.getFilterChainManager().createChain("/**", "anon");
Review Comment:
The PR description lists additional new tests (e.g.
"NullNormalizationBypassPocTest" and multiple new test cases) that do not
appear to be present in this change set. Either add the missing test
class/cases or update the PR description so it accurately reflects the included
coverage.
##########
web/src/main/java/org/apache/shiro/web/util/WebUtils.java:
##########
@@ -118,7 +118,11 @@ private WebUtils() {
* @return the path within the web application
*/
public static String getPathWithinApplication(HttpServletRequest request) {
- return normalize(removeSemicolon(getServletPath(request) +
getPathInfo(request)));
+ String path = normalize(removeSemicolon(getServletPath(request) +
getPathInfo(request)));
+ if (path == null) {
+ path = "/";
+ }
+ return path;
Review Comment:
When normalize(...) fails (returns null on "/../" at index 0), falling back
to "/" can cause the request to match a dedicated "/" chain (often more
permissive) instead of the intended catch-all "/* *" chain. That can still
allow traversal-above-root requests to be evaluated under the wrong filter
chain if the container/framework later normalizes the path differently.
Consider returning the original (semicolon-stripped) raw path when
normalization fails (and only map empty raw paths to "/"), so that a configured
"/**" chain is selected without accidentally matching an exact "/" chain.
##########
web/src/main/java/org/apache/shiro/web/filter/mgt/PathMatchingFilterChainResolver.java:
##########
@@ -150,6 +150,16 @@ public FilterChain getChain(ServletRequest request,
ServletResponse response, Fi
}
}
+ // If no chain matched, fall back to the catch-all chain if one exists.
+ // This ensures global filters (e.g. InvalidRequestFilter) always run,
+ // even if the requestURI is null or doesn't match any pattern.
Review Comment:
This comment says the fallback applies when the requestURI "doesn't match
any pattern", but the condition only triggers when requestURI is null/empty.
Since this block already executes only after no match, the wording is
misleading and makes it harder to understand the intended safety net semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
