Re: [PR] [#2837] - Null path normalization in getPathWithinApplication causes filter chain bypass [shiro]

["Copilot (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@shiro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org