Improved: Check parameters passed in URLs (OFBIZ-13295)

Better completely bypass "Prevents stream exploitation" block in
ControlFilter.java

Also better uses the token bypassPreventsStreamExploitation in
ControlFilterTests

Author: JacquesLeRoux

framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java

@@ -170,33 +170,32 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
         String context = req.getContextPath();
         HttpSession session = req.getSession();
 
-        // Prevents stream exploitation
-
         if (!isControlFilterTests()) {
+            // Prevents stream exploitation
             UrlServletHelper.setRequestAttributes(req, null, req.getServletContext());
-        }
-        Map<String, Object> parameters = UtilHttp.getParameterMap(req);
-        boolean reject = false;
-        if (!parameters.isEmpty()) {
-            for (String key : parameters.keySet()) {
-                Object object = parameters.get(key);
-                if (object.getClass().equals(String.class)
-                        || object instanceof Collection) {
-                    try {
-                        List<String> toCheck = object.getClass().equals(String.class)
-                                ? List.of((String) object)
-                                : UtilGenerics.checkCollection(object, String.class);
-                        reject = toCheck.stream()
-                                .anyMatch(val -> val.contains("<"));
-                    } catch (IllegalArgumentException e) {
-                        Debug.logWarning(e, MODULE);
-                        reject = true;
+            Map<String, Object> parameters = UtilHttp.getParameterMap(req);
+            boolean reject = false;
+            if (!parameters.isEmpty()) {
+                for (String key : parameters.keySet()) {
+                    Object object = parameters.get(key);
+                    if (object.getClass().equals(String.class)
+                            || object instanceof Collection) {
+                        try {
+                            List<String> toCheck = object.getClass().equals(String.class)
+                                    ? List.of((String) object)
+                                            : UtilGenerics.checkCollection(object, String.class);
+                            reject = toCheck.stream()
+                                    .anyMatch(val -> val.contains("<"));
+                        } catch (IllegalArgumentException e) {
+                            Debug.logWarning(e, MODULE);
+                            reject = true;
+                        }
                     }
                 }
-            }
-            if (reject) {
-                Debug.logError("For security reason this URL is not accepted", MODULE);
-                throw new RuntimeException("For security reason this URL is not accepted");
+                if (reject) {
+                    Debug.logError("For security reason this URL is not accepted", MODULE);
+                    throw new RuntimeException("For security reason this URL is not accepted");
+                }
             }
         }
 

framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java

@@ -58,7 +58,7 @@ public void setUp() {
 
     @Test
     public void filterWithExactAllowedPath() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("/foo");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo:/bar");
         when(req.getRequestURI()).thenReturn("/servlet/bar");
@@ -72,7 +72,7 @@ public void filterWithExactAllowedPath() throws Exception {
 
     @Test
     public void filterWithAllowedSubPath() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("/foo");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo:/bar");
         when(req.getRequestURI()).thenReturn("/servlet/bar/baz");
@@ -86,7 +86,7 @@ public void filterWithAllowedSubPath() throws Exception {
 
     @Test
     public void filterWithRedirection() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("/foo");
         when(config.getInitParameter("allowedPaths")).thenReturn("/bar:/baz");
         when(req.getRequestURI()).thenReturn("/missing/path");
@@ -99,7 +99,7 @@ public void filterWithRedirection() throws Exception {
 
     @Test
     public void filterWithURIredirection() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("http://example.org/foo");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo:/bar");
         when(req.getRequestURI()).thenReturn("/baz");
@@ -112,7 +112,7 @@ public void filterWithURIredirection() throws Exception {
 
     @Test
     public void bailsOutWithVariousErrorCodes() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
         when(req.getRequestURI()).thenReturn("/baz");
 
@@ -143,7 +143,7 @@ public void bailsOutWithVariousErrorCodes() throws Exception {
 
     @Test
     public void redirectAllAllowed() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("/bar");
         when(config.getInitParameter("forceRedirectAll")).thenReturn("Y");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
@@ -157,7 +157,7 @@ public void redirectAllAllowed() throws Exception {
 
     @Test
     public void redirectAllNotAllowed() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("/bar");
         when(config.getInitParameter("forceRedirectAll")).thenReturn("Y");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
@@ -171,7 +171,7 @@ public void redirectAllNotAllowed() throws Exception {
 
     @Test
     public void redirectAllRecursive() throws Exception {
-        System.setProperty("ControlFilterTests", "runsAfterControlFilter");
+        System.setProperty("ControlFilterTests", "bypassPreventsStreamExploitation");
         when(config.getInitParameter("redirectPath")).thenReturn("/foo");
         when(config.getInitParameter("forceRedirectAll")).thenReturn("Y");
         when(config.getInitParameter("allowedPaths")).thenReturn("/foo");