[GitHub] [sling-org-apache-sling-engine] rombert commented on a change in pull request #15: SLING-10225: don't allow path segements with only dots

[GitBox <gi...@apache.org>]

rombert commented on a change in pull request #15:
URL: https://github.com/apache/sling-org-apache-sling-engine/pull/15#discussion_r642522526



##########
File path: src/main/java/org/apache/sling/engine/impl/request/RequestData.java
##########
@@ -582,28 +582,17 @@ public static void service(SlingHttpServletRequest request,
     }
 
     /*
-     * Returns true if and only if path contains no consecutive dots
-     * except "/..".
-     *
-     * @param path The path of request object
-     * @return true if path contains no consecutive dots except "/..", false otherwise
+     * Don't allow path segments that contain only dots or a mix of dots and %5B.
+     * Additionally, check that we didn't have an empty selector from a dot replacement.
      */
-    static boolean isValidRequest(String path) {
-        boolean isValidRequest = true;
-        if (path.contains("...")) { //any occurrence "..." will mark request invalid
-            isValidRequest = false;
-        } else {
-            //consecutive dots will be treated as Invalid request except "/.."
-            int doubleDotIndex = path.indexOf(CONSECUTIVE_DOTS);
-            while (doubleDotIndex >= 0) {
-                if (doubleDotIndex == 0 || path.charAt(doubleDotIndex - 1) != '/') {//doubleDotIndex == 0 When path start with ..
-                    isValidRequest = false;
-                    break;
-                }
-                doubleDotIndex = path.indexOf(CONSECUTIVE_DOTS, doubleDotIndex + 2);
+    static boolean isValidRequest(SlingHttpServletRequest request) {
+        RequestPathInfo info = request.getRequestPathInfo();
+        for (String selector : info.getSelectors()) {
+            if (selector.trim().isEmpty()) {
+                return false;
             }
         }
-        return isValidRequest;
+        return !info.getResourcePath().matches(".*/\\[*\\.\\[*\\.[\\[,\\.]*/.*|.*/\\[*\\.\\[*\\.[\\[,\\.]*$");

Review comment:
       I am not sure what this does, so a comment would help. You might want to extract it as a static/instance field and use `Pattern.compile`, it should be a bit faster.

##########
File path: src/test/java/org/apache/sling/engine/impl/request/RequestDataTest.java
##########
@@ -150,11 +196,520 @@ public void testValidRequest() {
         assertValidRequest(true, "/path");
     }
 
-    private static void assertValidRequest(boolean expected, String path) {
+    private static void assertValidRequest(boolean expected, final String path) {
         assertEquals(
                 "Expected " + expected + " for " + path,
                 expected,
-                RequestData.isValidRequest(path)
+                RequestData.isValidRequest(new SlingHttpServletRequest(){

Review comment:
       Could we make this more compact? As it is it's a lot of effort to understand why this was needed. I think we already import jmock so that would be an option. We also have a `MockSlingHttpServletRequest` class in the servlet helpers module which you can use, but not sure if we get in trouble with cyclic dependencies.




-- 
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.

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