Fix #408: validate characters of document field names (#429)

stargate · May 16, 2023 · 6ca38e7 · 6ca38e7
1 parent f9f63e4
commit 6ca38e7
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 17 deletions.
diff --git a/src/main/java/io/stargate/sgv2/jsonapi/config/constants/DocumentConstants.java b/src/main/java/io/stargate/sgv2/jsonapi/config/constants/DocumentConstants.java
@@ -1,12 +1,18 @@
 package io.stargate.sgv2.jsonapi.config.constants;
 
 import io.stargate.sgv2.jsonapi.api.model.command.clause.filter.JsonType;
+import java.util.regex.Pattern;
 
 public interface DocumentConstants {
   /** Names of "special" fields in Documents */
   interface Fields {
     /** Primary key for Documents stored; has special handling for many operations. */
     String DOC_ID = "_id";
+
+    // Current definition of valid JSON API names: note that this only validates
+    // characters, not length limits (nor empty nor "too long" allowed but validated
+    // separately)
+    Pattern VALID_NAME_PATTERN = Pattern.compile("[a-zA-Z0-9_]*");
   }
 
   interface KeyTypeId {

diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/ErrorCode.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/ErrorCode.java
@@ -37,6 +37,8 @@ public enum ErrorCode {
 
   SHRED_DOC_LIMIT_VIOLATION("Document size limitation violated"),
 
+  SHRED_DOC_KEY_NAME_VIOLATION("Document key name constraints violated"),
+
   SHRED_BAD_EJSON_VALUE("Bad EJSON value"),
 
   INVALID_FILTER_EXPRESSION("Invalid filter expression"),

diff --git a/src/main/java/io/stargate/sgv2/jsonapi/service/shredding/Shredder.java b/src/main/java/io/stargate/sgv2/jsonapi/service/shredding/Shredder.java
@@ -11,6 +11,7 @@
 import io.stargate.sgv2.jsonapi.service.shredding.model.DocValueHasher;
 import io.stargate.sgv2.jsonapi.service.shredding.model.DocumentId;
 import io.stargate.sgv2.jsonapi.service.shredding.model.WritableShreddedDocument;
+import io.stargate.sgv2.jsonapi.util.JsonUtil;
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.Map;
@@ -229,17 +230,33 @@ private void validateObjectValue(DocumentLimitsConfig limits, JsonNode objectVal
     var it = objectValue.fields();
     while (it.hasNext()) {
       var entry = it.next();
-      final String key = entry.getKey();
-      if (key.length() > documentLimits.maxPropertyNameLength()) {
+      validateObjectKey(limits, entry.getKey(), entry.getValue());
+      validateDocValue(limits, entry.getValue(), depth);
+    }
+  }
+
+  private void validateObjectKey(DocumentLimitsConfig limits, String key, JsonNode value) {
+    if (key.length() > documentLimits.maxPropertyNameLength()) {
+      throw new JsonApiException(
+          ErrorCode.SHRED_DOC_LIMIT_VIOLATION,
+          String.format(
+              "%s: Property name length (%d) exceeds maximum allowed (%s)",
+              ErrorCode.SHRED_DOC_LIMIT_VIOLATION.getMessage(),
+              key.length(),
+              limits.maxPropertyNameLength()));
+    }
+    if (!DocumentConstants.Fields.VALID_NAME_PATTERN.matcher(key).matches()) {
+      // Special names are accepted in some cases: for now only one such case for
+      // Date values -- if more needed, will refactor. But for now inline:
+      if (JsonUtil.EJSON_VALUE_KEY_DATE.equals(key) && value.isValueNode()) {
+        ; // Fine, looks like legit Date value
+      } else {
         throw new JsonApiException(
-            ErrorCode.SHRED_DOC_LIMIT_VIOLATION,
+            ErrorCode.SHRED_DOC_KEY_NAME_VIOLATION,
             String.format(
-                "%s: Property name length (%d) exceeds maximum allowed (%s)",
-                ErrorCode.SHRED_DOC_LIMIT_VIOLATION.getMessage(),
-                key.length(),
-                limits.maxPropertyNameLength()));
+                "%s: Property name ('%s') contains character(s) not allowed",
+                ErrorCode.SHRED_DOC_KEY_NAME_VIOLATION.getMessage(), key));
       }
-      validateDocValue(limits, entry.getValue(), depth);
     }
   }
 

diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/shredding/ShredderDocLimitsTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/shredding/ShredderDocLimitsTest.java
@@ -16,6 +16,8 @@
 import org.apache.commons.lang3.RandomStringUtils;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 @QuarkusTest
 @TestProfile(NoGlobalResourcesTestProfile.Impl.class)
@@ -178,7 +180,7 @@ class ValidationDocAtomicSizeViolations {
     public void allowNotTooLongNames() {
       final ObjectNode doc = objectMapper.createObjectNode();
       doc.put("_id", 123);
-      doc.put("prop-123456789-123456789-123456789-123456789", true);
+      doc.put("prop_123456789_123456789_123456789_123456789", true);
       assertThat(shredder.shred(doc)).isNotNull();
     }
 
@@ -188,7 +190,7 @@ public void catchTooLongNames() {
       doc.put("_id", 123);
       ObjectNode ob = doc.putObject("subdoc");
       final String propName =
-          "property-with-way-too-long-name-123456789-123456789-123456789-123456789";
+          "property_with_way_too_long_name_123456789_123456789_123456789_123456789";
       ob.put(propName, true);
 
       Exception e = catchException(() -> shredder.shred(doc));
@@ -205,11 +207,30 @@ public void catchTooLongNames() {
                   + ")");
     }
 
+    @ParameterizedTest
+    @ValueSource(strings = {"$function", "dot.ted", "index[1]"})
+    public void catchInvalidFieldName(String invalidName) {
+      final ObjectNode doc = objectMapper.createObjectNode();
+      doc.put("_id", 123);
+      doc.put(invalidName, 123456);
+
+      Exception e = catchException(() -> shredder.shred(doc));
+      assertThat(e)
+          .isNotNull()
+          .isInstanceOf(JsonApiException.class)
+          .hasFieldOrPropertyWithValue("errorCode", ErrorCode.SHRED_DOC_KEY_NAME_VIOLATION)
+          .hasMessageStartingWith(ErrorCode.SHRED_DOC_KEY_NAME_VIOLATION.getMessage())
+          .hasMessageEndingWith(
+              "Document key name constraints violated: Property name ('"
+                  + invalidName
+                  + "') contains character(s) not allowed");
+    }
+
     @Test
     public void allowNotTooLongStringValues() {
       final ObjectNode doc = objectMapper.createObjectNode();
       doc.put("_id", 123);
-      // Max is 16_000 so do bit less
+      // Max is 16_000 so do a bit less
       doc.put("text", RandomStringUtils.randomAscii(12_000));
       assertThat(shredder.shred(doc)).isNotNull();
     }

diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/shredding/ShredderTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/shredding/ShredderTest.java
@@ -43,7 +43,7 @@ public void shredSimpleWithId() throws Exception {
                       { "_id" : "abc",
                         "name" : "Bob",
                         "values" : [ 1, 2 ],
-                        "[extra.stuff]" : true,
+                        "extra_stuff" : true,
                         "nullable" : null
                       }
                       """;
@@ -57,7 +57,7 @@ public void shredSimpleWithId() throws Exception {
               JsonPath.from("values"),
               JsonPath.from("values.0", true),
               JsonPath.from("values.1", true),
-              JsonPath.from("[extra.stuff]"),
+              JsonPath.from("extra_stuff"),
               JsonPath.from("nullable"));
 
       // First verify paths
@@ -76,7 +76,7 @@ public void shredSimpleWithId() throws Exception {
               "name SBob",
               "values N1",
               "values N2",
-              "[extra.stuff] B1",
+              "extra_stuff B1",
               "nullable Z",
               "values.0 N1",
               "values.1 N2");
@@ -90,7 +90,7 @@ public void shredSimpleWithId() throws Exception {
 
       // Then atomic value containers
       assertThat(doc.queryBoolValues())
-          .isEqualTo(Collections.singletonMap(JsonPath.from("[extra.stuff]"), Boolean.TRUE));
+          .isEqualTo(Collections.singletonMap(JsonPath.from("extra_stuff"), Boolean.TRUE));
       Map<JsonPath, BigDecimal> expNums = new LinkedHashMap<>();
       expNums.put(JsonPath.from("values.0", true), BigDecimal.valueOf(1));
       expNums.put(JsonPath.from("values.1", true), BigDecimal.valueOf(2));
@@ -271,8 +271,8 @@ public void badEJSONUnrecognized() {
 
       assertThat(t)
           .isNotNull()
-          .hasMessage("Bad EJSON value: unrecognized type '$unknownType' (path 'value')")
-          .hasFieldOrPropertyWithValue("errorCode", ErrorCode.SHRED_BAD_EJSON_VALUE);
+          .hasMessageStartingWith("Document key name constraints violated")
+          .hasFieldOrPropertyWithValue("errorCode", ErrorCode.SHRED_DOC_KEY_NAME_VIOLATION);
     }
   }