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:
[email protected]
