Support hyphen in identifier names (#246)

Author: suddendust

document-store/src/main/java/org/hypertrace/core/documentstore/postgres/utils/BasicPostgresSecurityValidator.java

@@ -43,50 +43,18 @@ public class BasicPostgresSecurityValidator implements PostgresSecurityValidator
    * Documentation</a>
    */
   private static final String DEFAULT_IDENTIFIER_PATTERN =
-      "^[a-zA-Z_][a-zA-Z0-9_]*(\\.[a-zA-Z_][a-zA-Z0-9_]*)*$";
-
-  /**
-   * Default pattern for JSON field names within JSONB columns.
-   *
-   * <p>Pattern: {@code ^[a-zA-Z0-9_]+$}
-   *
-   * <p><b>Allowed:</b>
-   *
-   * <ul>
-   *   <li>Can start with: letter (a-z, A-Z), digit (0-9), or underscore (_)
-   *   <li>Can contain: letters (a-z, A-Z), digits (0-9), underscores (_)
-   *   <li>Examples: {@code "brand"}, {@code "1st_choice"}, {@code "field123"}, {@code "_private"}
-   * </ul>
-   *
-   * <p><b>Not allowed:</b>
-   *
-   * <ul>
-   *   <li>Hyphens: {@code "field-name"}
-   *   <li>Dots: {@code "field.name"} (use path segments instead: ["field", "name"])
-   *   <li>Spaces: {@code "my field"}
-   *   <li>Special characters: {@code "field@name"}, {@code "field#name"}
-   *   <li>Quotes: {@code "field\"name"}, {@code "field'name"}
-   *   <li>Semicolons: {@code "field; DROP TABLE"}
-   *   <li>SQL operators: {@code "field\" OR \"1\"=\"1"}
-   * </ul>
-   *
-   * <p><b>Note:</b> More permissive than identifier pattern to support flexible JSON schemas where
-   * field names may start with numbers.
-   */
-  private static final String DEFAULT_JSON_FIELD_PATTERN = "^[a-zA-Z0-9_]+$";
+      "^[a-zA-Z_][a-zA-Z0-9_-]*(\\.[a-zA-Z_][a-zA-Z0-9_-]*)*$";
 
   /** Default instance with hardcoded values for convenient static access. */
   private static final BasicPostgresSecurityValidator DEFAULT =
       new BasicPostgresSecurityValidator(
           DEFAULT_MAX_IDENTIFIER_LENGTH,
           DEFAULT_MAX_JSON_FIELD_LENGTH,
           DEFAULT_MAX_JSON_PATH_DEPTH,
-          DEFAULT_IDENTIFIER_PATTERN,
-          DEFAULT_JSON_FIELD_PATTERN);
+          DEFAULT_IDENTIFIER_PATTERN);
 
   // Instance variables for configured limits
   private final Pattern validIdentifier;
-  private final Pattern validJsonField;
   private final int maxIdentifierLength;
   private final int maxJsonFieldLength;
   private final int maxJsonPathDepth;
@@ -105,13 +73,11 @@ private BasicPostgresSecurityValidator(
       int maxIdentifierLength,
       int maxJsonFieldLength,
       int maxJsonPathDepth,
-      String identifierPattern,
-      String jsonFieldPattern) {
+      String identifierPattern) {
     this.maxIdentifierLength = maxIdentifierLength;
     this.maxJsonFieldLength = maxJsonFieldLength;
     this.maxJsonPathDepth = maxJsonPathDepth;
     this.validIdentifier = Pattern.compile(identifierPattern);
-    this.validJsonField = Pattern.compile(jsonFieldPattern);
   }
 
   @Override
@@ -163,7 +129,7 @@ public void validateJsonPath(List<String> path) {
                 field, i, maxJsonFieldLength));
       }
 
-      if (!validJsonField.matcher(field).matches()) {
+      if (!validIdentifier.matcher(field).matches()) {
         throw new SecurityException(
             String.format(
                 "JSON field '%s' at index %d contains invalid characters. "

document-store/src/test/java/org/hypertrace/core/documentstore/expression/impl/JsonIdentifierExpressionSecurityTest.java

@@ -7,48 +7,51 @@
 import java.util.List;
 import org.junit.jupiter.api.Test;
 
-/** Security tests for JsonIdentifierExpression to ensure SQL injection prevention. */
 public class JsonIdentifierExpressionSecurityTest {
 
-  // ===== Valid Expressions =====
-
   @Test
-  void testValidExpression_SimpleField() {
+  void testValidExpressionSimpleField() {
     assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "brand"));
   }
 
   @Test
-  void testValidExpression_NestedField() {
+  void testValidExpressionNestedField() {
     assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "seller", "name"));
   }
 
   @Test
-  void testValidExpression_DeeplyNested() {
+  void testValidExpressionDeeplyNested() {
     assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "seller", "address", "city"));
   }
 
   @Test
-  void testValidExpression_WithNumbers() {
+  void testValidExpressionWithNumbers() {
     assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "field123"));
-    assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "1st_choice"));
+    assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "field_1"));
   }
 
   @Test
-  void testValidExpression_WithUnderscore() {
+  void testInvalidExpressionJsonPathStartsWithNumber() {
+    SecurityException ex =
+        assertThrows(
+            SecurityException.class, () -> JsonIdentifierExpression.of("props", "1st_choice"));
+    assertTrue(ex.getMessage().contains("invalid characters"));
+  }
+
+  @Test
+  void testValidExpressionWithUnderscore() {
     assertDoesNotThrow(() -> JsonIdentifierExpression.of("_internal", "field"));
     assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "_private"));
   }
 
   @Test
-  void testValidExpression_UsingListConstructor() {
+  void testValidExpressionUsingListConstructor() {
     assertDoesNotThrow(
         () -> JsonIdentifierExpression.of("props", List.of("seller", "address", "city")));
   }
 
-  // ===== Invalid Column Names =====
-
   @Test
-  void testInvalidExpression_ColumnName_DropTable() {
+  void testInvalidExpressionColumnNameDropTable() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -57,49 +60,39 @@ void testInvalidExpression_ColumnName_DropTable() {
   }
 
   @Test
-  void testInvalidExpression_ColumnName_WithQuote() {
+  void testInvalidExpressionColumnNameWithQuote() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("props\"name", "brand"));
     assertTrue(ex.getMessage().contains("invalid"));
   }
 
   @Test
-  void testInvalidExpression_ColumnName_WithSemicolon() {
+  void testInvalidExpressionColumnNameWithSemicolon() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("props;SELECT", "brand"));
     assertTrue(ex.getMessage().contains("invalid"));
   }
 
   @Test
-  void testInvalidExpression_ColumnName_StartsWithNumber() {
+  void testInvalidExpressionColumnNameStartsWithNumber() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("123props", "brand"));
     assertTrue(ex.getMessage().contains("Must start with a letter or underscore"));
   }
 
   @Test
-  void testInvalidExpression_ColumnName_WithHyphen() {
-    SecurityException ex =
-        assertThrows(
-            SecurityException.class, () -> JsonIdentifierExpression.of("my-column", "brand"));
-    assertTrue(ex.getMessage().contains("invalid"));
-  }
-
-  @Test
-  void testInvalidExpression_ColumnName_WithSpace() {
+  void testInvalidExpressionColumnNameWithSpace() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("my column", "brand"));
     assertTrue(ex.getMessage().contains("invalid"));
   }
 
-  // ===== Invalid JSON Paths =====
-
   @Test
-  void testInvalidExpression_JsonPath_WithQuote() {
+  void testInvalidExpressionJsonPathWithQuote() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -108,47 +101,43 @@ void testInvalidExpression_JsonPath_WithQuote() {
   }
 
   @Test
-  void testInvalidExpression_JsonPath_WithDoubleQuote() {
+  void testInvalidExpressionJsonPathWithDoubleQuote() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("props", "name\"--"));
     assertTrue(ex.getMessage().contains("invalid characters"));
   }
 
   @Test
-  void testInvalidExpression_JsonPath_WithSemicolon() {
+  void testInvalidExpressionJsonPathWithSemicolon() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("props", "field; DROP"));
     assertTrue(ex.getMessage().contains("invalid characters"));
   }
 
   @Test
-  void testInvalidExpression_JsonPath_WithHyphen() {
-    SecurityException ex =
-        assertThrows(
-            SecurityException.class, () -> JsonIdentifierExpression.of("props", "field-name"));
-    assertTrue(ex.getMessage().contains("invalid characters"));
+  void testValidExpressionJsonPathWithHyphen() {
+    assertDoesNotThrow(() -> JsonIdentifierExpression.of("customAttribute", "repo-url"));
+    assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "user-id"));
   }
 
   @Test
-  void testInvalidExpression_JsonPath_WithDot() {
-    SecurityException ex =
-        assertThrows(
-            SecurityException.class, () -> JsonIdentifierExpression.of("props", "field.name"));
-    assertTrue(ex.getMessage().contains("invalid characters"));
+  void testValidExpressionJsonPathWithDot() {
+    assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "field.name"));
+    assertDoesNotThrow(() -> JsonIdentifierExpression.of("props", "user.address.city"));
   }
 
   @Test
-  void testInvalidExpression_JsonPath_WithSpace() {
+  void testInvalidExpressionJsonPathWithSpace() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("props", "field name"));
     assertTrue(ex.getMessage().contains("invalid characters"));
   }
 
   @Test
-  void testInvalidExpression_JsonPath_EmptyElement() {
+  void testInvalidExpression_sonPathEmptyElement() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -157,7 +146,7 @@ void testInvalidExpression_JsonPath_EmptyElement() {
   }
 
   @Test
-  void testInvalidExpression_JsonPath_TooDeep() {
+  void testInvalidExpressionJsonPathTooDeep() {
     String[] deepPath = new String[11]; // Max is 10
     for (int i = 0; i < 11; i++) {
       deepPath[i] = "level" + i;
@@ -167,10 +156,8 @@ void testInvalidExpression_JsonPath_TooDeep() {
     assertTrue(ex.getMessage().contains("exceeds maximum depth"));
   }
 
-  // ===== Real-world Attack Scenarios =====
-
   @Test
-  void testAttackScenario_SqlCommentInjection() {
+  void testAttackScenarioSqlCommentInjection() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -179,7 +166,7 @@ void testAttackScenario_SqlCommentInjection() {
   }
 
   @Test
-  void testAttackScenario_UnionSelect() {
+  void testAttackScenarioUnionSelect() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -190,7 +177,7 @@ void testAttackScenario_UnionSelect() {
   }
 
   @Test
-  void testAttackScenario_OrTrueInjection() {
+  void testAttackScenarioOrTrueInjection() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -199,7 +186,7 @@ void testAttackScenario_OrTrueInjection() {
   }
 
   @Test
-  void testAttackScenario_NestedInjection() {
+  void testAttackScenarioNestedInjection() {
     SecurityException ex =
         assertThrows(
             SecurityException.class,
@@ -208,7 +195,7 @@ void testAttackScenario_NestedInjection() {
   }
 
   @Test
-  void testAttackScenario_SpecialCharacterCombination() {
+  void testAttackScenarioSpecialCharacterCombination() {
     SecurityException ex =
         assertThrows(
             SecurityException.class, () -> JsonIdentifierExpression.of("props", "field'\"`;DROP"));

document-store/src/test/java/org/hypertrace/core/documentstore/postgres/utils/PostgresSecurityValidatorTest.java

@@ -31,6 +31,11 @@ void testValidIdentifierWithNumbers() {
     assertDoesNotThrow(() -> validator.validateIdentifier("col_1"));
   }
 
+  @Test
+  void testValidIdentifierWithHyphens() {
+    assertDoesNotThrow(() -> validator.validateIdentifier("repo-url"));
+  }
+
   @Test
   void testInvalidIdentifierNull() {
     SecurityException ex =
@@ -75,13 +80,6 @@ void testInvalidIdentifierSqlInjection_Semicolon() {
     assertTrue(ex.getMessage().contains("invalid"));
   }
 
-  @Test
-  void testInvalidIdentifierHyphen() {
-    SecurityException ex =
-        assertThrows(SecurityException.class, () -> validator.validateIdentifier("field-name"));
-    assertTrue(ex.getMessage().contains("invalid"));
-  }
-
   @Test
   void testValidIdentifierWithDotNotation() {
     assertDoesNotThrow(() -> validator.validateIdentifier("field.name"));
@@ -142,7 +140,15 @@ void testValidJsonPathMultiLevel() {
   @Test
   void testValidJsonPathWithNumbers() {
     assertDoesNotThrow(() -> validator.validateJsonPath(List.of("field123")));
-    assertDoesNotThrow(() -> validator.validateJsonPath(List.of("1st_choice")));
+    assertDoesNotThrow(() -> validator.validateJsonPath(List.of("field_1")));
+  }
+
+  @Test
+  void testInvalidJsonPathStartsWithNumber() {
+    SecurityException ex =
+        assertThrows(
+            SecurityException.class, () -> validator.validateJsonPath(List.of("1st_choice")));
+    assertTrue(ex.getMessage().contains("invalid characters"));
   }
 
   @Test
@@ -211,19 +217,15 @@ void testInvalidJsonPathSqlInjectionSemicolon() {
   }
 
   @Test
-  void testInvalidJsonPathHyphen() {
-    SecurityException ex =
-        assertThrows(
-            SecurityException.class, () -> validator.validateJsonPath(List.of("field-name")));
-    assertTrue(ex.getMessage().contains("invalid characters"));
+  void testValidJsonPathWithHyphen() {
+    assertDoesNotThrow(() -> validator.validateJsonPath(List.of("field-name")));
+    assertDoesNotThrow(() -> validator.validateJsonPath(List.of("user-id")));
   }
 
   @Test
-  void testInvalidJsonPathDot() {
-    SecurityException ex =
-        assertThrows(
-            SecurityException.class, () -> validator.validateJsonPath(List.of("field.name")));
-    assertTrue(ex.getMessage().contains("invalid characters"));
+  void testValidJsonPathWithDotNotation() {
+    assertDoesNotThrow(() -> validator.validateJsonPath(List.of("field.name")));
+    assertDoesNotThrow(() -> validator.validateJsonPath(List.of("user.address.city")));
   }
 
   @Test