diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/utils/BasicPostgresSecurityValidator.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/utils/BasicPostgresSecurityValidator.java index e126bf98..4f3178f7 100644 --- a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/utils/BasicPostgresSecurityValidator.java +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/utils/BasicPostgresSecurityValidator.java @@ -43,37 +43,7 @@ public class BasicPostgresSecurityValidator implements PostgresSecurityValidator * Documentation */ 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. - * - *

Pattern: {@code ^[a-zA-Z0-9_]+$} - * - *

Allowed: - * - *

- * - *

Not allowed: - * - *

- * - *

Note: 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 = @@ -81,12 +51,10 @@ public class BasicPostgresSecurityValidator implements PostgresSecurityValidator 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 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. " diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/expression/impl/JsonIdentifierExpressionSecurityTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/expression/impl/JsonIdentifierExpressionSecurityTest.java index 7b8048f8..d2ab7831 100644 --- a/document-store/src/test/java/org/hypertrace/core/documentstore/expression/impl/JsonIdentifierExpressionSecurityTest.java +++ b/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,7 +60,7 @@ void testInvalidExpression_ColumnName_DropTable() { } @Test - void testInvalidExpression_ColumnName_WithQuote() { + void testInvalidExpressionColumnNameWithQuote() { SecurityException ex = assertThrows( SecurityException.class, () -> JsonIdentifierExpression.of("props\"name", "brand")); @@ -65,7 +68,7 @@ void testInvalidExpression_ColumnName_WithQuote() { } @Test - void testInvalidExpression_ColumnName_WithSemicolon() { + void testInvalidExpressionColumnNameWithSemicolon() { SecurityException ex = assertThrows( SecurityException.class, () -> JsonIdentifierExpression.of("props;SELECT", "brand")); @@ -73,7 +76,7 @@ void testInvalidExpression_ColumnName_WithSemicolon() { } @Test - void testInvalidExpression_ColumnName_StartsWithNumber() { + void testInvalidExpressionColumnNameStartsWithNumber() { SecurityException ex = assertThrows( SecurityException.class, () -> JsonIdentifierExpression.of("123props", "brand")); @@ -81,25 +84,15 @@ void testInvalidExpression_ColumnName_StartsWithNumber() { } @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,7 +101,7 @@ void testInvalidExpression_JsonPath_WithQuote() { } @Test - void testInvalidExpression_JsonPath_WithDoubleQuote() { + void testInvalidExpressionJsonPathWithDoubleQuote() { SecurityException ex = assertThrows( SecurityException.class, () -> JsonIdentifierExpression.of("props", "name\"--")); @@ -116,7 +109,7 @@ void testInvalidExpression_JsonPath_WithDoubleQuote() { } @Test - void testInvalidExpression_JsonPath_WithSemicolon() { + void testInvalidExpressionJsonPathWithSemicolon() { SecurityException ex = assertThrows( SecurityException.class, () -> JsonIdentifierExpression.of("props", "field; DROP")); @@ -124,23 +117,19 @@ void testInvalidExpression_JsonPath_WithSemicolon() { } @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")); @@ -148,7 +137,7 @@ void testInvalidExpression_JsonPath_WithSpace() { } @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")); diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/utils/PostgresSecurityValidatorTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/utils/PostgresSecurityValidatorTest.java index 83ae9ed9..1abad456 100644 --- a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/utils/PostgresSecurityValidatorTest.java +++ b/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