Improve validation and escaping of number/group visiting parameters

Also use explicit `==` equality operator instead of `=` in generated expression.

3 files changed, 94 insertions, 16 deletions

diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/OperationHandlerImpl.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/OperationHandlerImpl.java
index cb4d1b9feed..c0ce3e84232 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/OperationHandlerImpl.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/OperationHandlerImpl.java
@@ -355,12 +355,6 @@ public class OperationHandlerImpl implements OperationHandler {
                 clusterListToString(clusters) + " not " + wantedCluster.get() + ". Please select a valid vespa cluster.", RestUri.apiErrorCodes.MISSING_CLUSTER));
     }
 
-    // Based on resolveClusterRoute in VdsVisit, protected for testability
-    // TODO remove in favor of resolveClusterDef
-    protected static String resolveClusterRoute(Optional<String> wantedCluster, List<ClusterDef> clusters) throws RestApiException {
-        return clusterDefToRoute(resolveClusterDef(wantedCluster, clusters));
-    }
-
     protected static String clusterDefToRoute(ClusterDef clusterDef) {
         return "[Storage:cluster=" + clusterDef.getName() + ";clusterconfigid=" + clusterDef.getConfigId() + "]";
     }
diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java
index bb9387dc6e0..8639d4fad87 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/document/restapi/resource/RestApi.java
@@ -278,27 +278,60 @@ public class RestApi extends LoggingRequestHandler {
 
         return optionsBuilder.build();
     }
-    
-    private HttpResponse handleVisit(RestUri restUri, HttpRequest request) throws RestApiException {
+
+    /**
+     * Escapes all single quotes in input string.
+     * @param original non-escaped string that may contain single quotes
+     * @return original if no quotes to escaped were found, otherwise a quote-escaped string
+     */
+    private static String singleQuoteEscapedString(String original) {
+        if (original.indexOf('\'') == -1) {
+            return original;
+        }
+        StringBuilder builder = new StringBuilder(original.length() + 1);
+        for (int i = 0; i < original.length(); ++i) {
+            char c = original.charAt(i);
+            if (c != '\'') {
+                builder.append(c);
+            } else {
+                builder.append("\\'");
+            }
+        }
+        return builder.toString();
+    }
+
+    private static long parseAndValidateVisitNumericId(String value) {
+        try {
+            return Long.parseLong(value);
+        } catch (NumberFormatException e) {
+            throw new BadRequestParameterException(SELECTION, "Failed to parse numeric part of selection URI");
+        }
+    }
+
+    private static String documentSelectionFromRequest(RestUri restUri, HttpRequest request) throws BadRequestParameterException {
         String documentSelection = Optional.ofNullable(request.getProperty(SELECTION)).orElse("");
         if (restUri.getGroup().isPresent() && ! restUri.getGroup().get().value.isEmpty()) {
             if (! documentSelection.isEmpty()) {
                 // TODO why is this restriction in place? Document selection allows composition of location predicate and other expressions
-                return Response.createErrorResponse(
-                        400,
-                        "Visiting does not support setting value for group/value in combination with expression, try using only expression parameter instead.",
-                        restUri,
-                        RestUri.apiErrorCodes.GROUP_AND_EXPRESSION_ERROR);
+                throw new BadRequestParameterException(SELECTION, "Visiting does not support setting value for group/value in combination with expression, try using only expression parameter instead.");
             }
             RestUri.Group group = restUri.getGroup().get();
             if (group.name == 'n') {
-                documentSelection = "id.user=" + group.value;
+                documentSelection = String.format("id.user==%d", parseAndValidateVisitNumericId(group.value));
             } else {
-                documentSelection = "id.group='" + group.value + "'";
+                // TODO first pass through Text.validateTextString? Cannot create doc IDs that don't match that anyway...
+                documentSelection = String.format("id.group=='%s'", singleQuoteEscapedString(group.value));
             }
         }
+        // TODO try to preemptively parse sub expression to ensure it is complete
+        return documentSelection;
+    }
+
+    private HttpResponse handleVisit(RestUri restUri, HttpRequest request) throws RestApiException {
+        String documentSelection;
         OperationHandler.VisitOptions options;
         try {
+            documentSelection = documentSelectionFromRequest(restUri, request);
             options = visitOptionsFromRequest(request);
         } catch (BadRequestParameterException e) {
             return createInvalidParameterResponse(e.getParameter(), e.getMessage());
diff --git a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java
index eb06af41348..be8915496ac 100644
--- a/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java
+++ b/vespaclient-container-plugin/src/test/java/com/yahoo/document/restapi/resource/RestApiTest.java
@@ -24,6 +24,9 @@ import org.junit.Ignore;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Paths;
 
 import static org.hamcrest.core.Is.is;
@@ -291,7 +294,7 @@ public class RestApiTest {
     }
 
     String visit_test_uri_selection_rewrite = "/document/v1/namespace/document-type/group/abc?continuation=abc";
-    String visit_test_response_selection_rewrite = "doc selection: 'id.group='abc''";
+    String visit_test_response_selection_rewrite = "doc selection: 'id.group=='abc''";
 
 
     @Test
@@ -302,6 +305,54 @@ public class RestApiTest {
         assertThat(rest, containsString(visit_test_response_selection_rewrite));
     }
 
+    private static String encoded(String original) {
+        try {
+            return URLEncoder.encode(original, StandardCharsets.UTF_8.name());
+        } catch (UnsupportedEncodingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private String performV1RestCall(String pathSuffix) {
+        try {
+            Request request = new Request(String.format("http://localhost:%s/document/v1/namespace/document-type/%s",
+                    getFirstListenPort(), pathSuffix));
+            HttpGet get = new HttpGet(request.getUri());
+            return doRest(get);
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private void assertResultingDocumentSelection(String suffix, String expected) {
+        String output = performV1RestCall(suffix);
+        assertThat(output, containsString(String.format("doc selection: '%s'", expected)));
+    }
+
+    private void assertGroupDocumentSelection(String group, String expected) {
+        assertResultingDocumentSelection("group/" + encoded(group), expected);
+    }
+
+    @Test
+    public void group_strings_are_escaped() {
+        assertGroupDocumentSelection("'", "id.group=='\\''");
+        assertGroupDocumentSelection("hello 'world'", "id.group=='hello \\'world\\''");
+        assertGroupDocumentSelection("' goodbye moon", "id.group=='\\' goodbye moon'");
+    }
+
+    private void assertNumericIdFailsParsing(String id) {
+        String output = performV1RestCall(String.format("number/%s", encoded(id)));
+        assertThat(output, containsString("Failed to parse numeric part of selection URI"));
+    }
+
+    @Test
+    public void invalid_numeric_id_returns_error() {
+        assertNumericIdFailsParsing("123a");
+        assertNumericIdFailsParsing("a123");
+        assertNumericIdFailsParsing("0x1234");
+        assertNumericIdFailsParsing("\u0000");
+    }
+
     @Test
     public void wanted_document_count_returned_parameter_is_propagated() throws IOException {
         Request request = new Request(String.format("http://localhost:%s/document/v1/namespace/document-type/docid/?wantedDocumentCount=321", getFirstListenPort()));