Ensure selection parameter sub-expression is complete and valid

Also add validation that specified group only contains allowed text
characters. This mirrors the check that is done for group names in
IDs when documents are originally fed.

2 files changed, 42 insertions, 2 deletions

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 1dedbd857c3..49b27eba613 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
@@ -21,10 +21,13 @@ import com.yahoo.document.restapi.OperationHandlerImpl;
 import com.yahoo.document.restapi.Response;
 import com.yahoo.document.restapi.RestApiException;
 import com.yahoo.document.restapi.RestUri;
+import com.yahoo.document.select.DocumentSelector;
+import com.yahoo.document.select.parser.ParseException;
 import com.yahoo.documentapi.messagebus.MessageBusDocumentAccess;
 import com.yahoo.documentapi.messagebus.MessageBusParams;
 import com.yahoo.documentapi.messagebus.loadtypes.LoadTypeSet;
 import com.yahoo.metrics.simple.MetricReceiver;
+import com.yahoo.text.Text;
 import com.yahoo.vespa.config.content.LoadTypeConfig;
 import com.yahoo.vespaxmlparser.VespaXMLFeedReader;
 
@@ -312,14 +315,31 @@ public class RestApi extends LoggingRequestHandler {
         if (group.name == 'n') {
             return String.format("id.user==%d", parseAndValidateVisitNumericId(group.value));
         } else {
-            // TODO first pass through Text.validateTextString? Cannot create doc IDs that don't match that anyway...
+            // Cannot feed documents with groups that don't pass this test, so it makes sense
+            // to enforce this symmetry when trying to retrieve them as well.
+            Text.validateTextString(group.value).ifPresent(codepoint -> {
+                throw new BadRequestParameterException(SELECTION, String.format(
+                        "Failed to parse group part of selection URI; contains invalid text code point U%04X", codepoint));
+            });
             return String.format("id.group=='%s'", singleQuoteEscapedString(group.value));
         }
     }
 
+    private static void validateDocumentSelectionSyntax(String expression) {
+        try {
+            new DocumentSelector(expression);
+        } catch (ParseException e) {
+            throw new BadRequestParameterException(SELECTION, String.format("Failed to parse expression given in 'selection'" +
+                    " parameter. Must be a complete and valid sub-expression. Error: %s", e.getMessage()));
+        }
+    }
+
     private static String documentSelectionFromRequest(RestUri restUri, HttpRequest request) throws BadRequestParameterException {
-        // TODO try to preemptively parse sub expression to ensure it is complete
         String documentSelection = Optional.ofNullable(request.getProperty(SELECTION)).orElse("");
+        if (!documentSelection.isEmpty()) {
+            // Ensure that the selection parameter sub-expression is complete and valid by itself.
+            validateDocumentSelectionSyntax(documentSelection);
+        }
         if (restUri.getGroup().isPresent() && ! restUri.getGroup().get().value.isEmpty()) {
             String locationSubExpression = validateAndBuildLocationSubExpression(restUri.getGroup().get());
             if (documentSelection.isEmpty()) {
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 ed7935b698b..14d8239ba41 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
@@ -340,6 +340,12 @@ public class RestApiTest {
     }
 
     @Test
+    public void non_text_group_string_character_returns_error() {
+        String output = performV1RestCall(String.format("group/%s", encoded("\u001f")));
+        assertThat(output, containsString("Failed to parse group part of selection URI; contains invalid text code point U001F"));
+    }
+
+    @Test
     public void can_specify_numeric_id_without_explicit_selection() {
         assertResultingDocumentSelection("number/1234", "id.user==1234");
     }
@@ -361,6 +367,20 @@ public class RestApiTest {
                 "id.group=='bar' and (3 != 4)");
     }
 
+    private void assertDocumentSelectionFailsParsing(String expression) {
+        String output = performV1RestCall(String.format("number/1234?selection=%s", encoded(expression)));
+        assertThat(output, containsString("Failed to parse expression given in 'selection' parameter. Must be a complete and valid sub-expression."));
+    }
+
+    // Make sure that typoing the selection parameter doesn't corrupt the entire selection expression
+    @Test
+    public void explicit_selection_sub_expression_is_validated_for_completeness() {
+        assertDocumentSelectionFailsParsing("1 +");
+        assertDocumentSelectionFailsParsing(") or true");
+        assertDocumentSelectionFailsParsing("((1 + 2)");
+        assertDocumentSelectionFailsParsing("true) or (true");
+    }
+
     @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()));