Validate regex in label rule creation/updation (#189)

* Validate regex in label rule creation/updation * Updated key condition validator * Move RegexValidator to validation-utils * spotlessApply * Addressed review comments
hypertrace · Dec 14, 2023 · 2f6e0ea · 2f6e0ea
1 parent 044e07f
commit 2f6e0ea
Show file tree

Hide file tree

Showing 11 changed files with 148 additions and 39 deletions.
diff --git a/alerting-config-service-impl/gradle.lockfile b/alerting-config-service-impl/gradle.lockfile
@@ -21,6 +21,7 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava=comp
 com.google.j2objc:j2objc-annotations:2.8=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java-util:3.24.1=runtimeClasspath
 com.google.protobuf:protobuf-java:3.24.1=compileClasspath,runtimeClasspath
+com.google.re2j:re2j:1.7=runtimeClasspath
 com.typesafe:config:1.4.2=compileClasspath,runtimeClasspath
 commons-io:commons-io:2.7=runtimeClasspath
 io.grpc:grpc-api:1.60.0=compileClasspath,runtimeClasspath

diff --git a/...roto/org/hypertrace/label/application/rule/config/service/v1/label_application_rule.proto b/...roto/org/hypertrace/label/application/rule/config/service/v1/label_application_rule.proto
@@ -35,6 +35,7 @@ message LabelApplicationRuleData {
   }
 
   message LeafCondition {
+    // only equals and regex are supported for key condition
     StringCondition key_condition = 1;
     oneof condition {
       StringCondition string_condition = 2;

diff --git a/label-application-rule-config-service-impl/build.gradle.kts b/label-application-rule-config-service-impl/build.gradle.kts
@@ -15,6 +15,7 @@ dependencies {
   implementation(commonLibs.protobuf.javautil)
   implementation(commonLibs.hypertrace.grpcutils.context)
   implementation(commonLibs.hypertrace.grpcutils.client)
+  implementation(commonLibs.google.re2j)
 
   annotationProcessor(commonLibs.lombok)
   compileOnly(commonLibs.lombok)

diff --git a/label-application-rule-config-service-impl/gradle.lockfile b/label-application-rule-config-service-impl/gradle.lockfile
@@ -22,6 +22,7 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava=comp
 com.google.j2objc:j2objc-annotations:2.8=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java-util:3.24.1=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java:3.24.1=compileClasspath,runtimeClasspath
+com.google.re2j:re2j:1.7=compileClasspath,runtimeClasspath
 com.typesafe:config:1.4.2=compileClasspath,runtimeClasspath
 commons-io:commons-io:2.7=runtimeClasspath
 io.grpc:grpc-api:1.60.0=compileClasspath,runtimeClasspath

diff --git a/...g/hypertrace/label/application/rule/config/service/LabelApplicationRuleValidatorImpl.java b/...g/hypertrace/label/application/rule/config/service/LabelApplicationRuleValidatorImpl.java
@@ -4,16 +4,15 @@
 import static org.hypertrace.config.validation.GrpcValidatorUtils.printMessage;
 import static org.hypertrace.config.validation.GrpcValidatorUtils.validateNonDefaultPresenceOrThrow;
 import static org.hypertrace.config.validation.GrpcValidatorUtils.validateRequestContextOrThrow;
-import static org.hypertrace.label.application.rule.config.service.v1.LabelApplicationRuleData.StringCondition.Operator.OPERATOR_MATCHES_IPS;
-import static org.hypertrace.label.application.rule.config.service.v1.LabelApplicationRuleData.StringCondition.Operator.OPERATOR_NOT_MATCHES_IPS;
 
 import com.google.protobuf.Message;
+import com.google.re2j.Matcher;
+import com.google.re2j.Pattern;
 import io.grpc.Status;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
+import org.hypertrace.config.validation.RegexValidator;
 import org.hypertrace.core.grpcutils.context.RequestContext;
 import org.hypertrace.label.application.rule.config.service.v1.CreateLabelApplicationRuleRequest;
 import org.hypertrace.label.application.rule.config.service.v1.DeleteLabelApplicationRuleRequest;
@@ -82,7 +81,7 @@ private void validateCondition(LabelApplicationRuleData.Condition condition) {
   }
 
   private void validateLeafCondition(LeafCondition leafCondition) {
-    validateStringCondition(leafCondition.getKeyCondition());
+    validateKeyStringCondition(leafCondition.getKeyCondition());
     switch (leafCondition.getConditionCase()) {
       case STRING_CONDITION:
         validateStringCondition(leafCondition.getStringCondition());
@@ -123,31 +122,71 @@ private void validateJsonCondition(JsonCondition jsonCondition) {
     }
   }
 
+  private void validateKeyStringCondition(StringCondition stringCondition) {
+    validateNonDefaultPresenceOrThrow(stringCondition, StringCondition.VALUE_FIELD_NUMBER);
+    switch (stringCondition.getOperator()) {
+      case OPERATOR_EQUALS:
+        break;
+      case OPERATOR_MATCHES_REGEX:
+        final String keyPattern = stringCondition.getValue();
+        final Status regexValidationStatus = RegexValidator.validate(keyPattern);
+        if (!regexValidationStatus.isOk()) {
+          throw regexValidationStatus
+              .withDescription(
+                  String.format(
+                      "Invalid regex for key : %s for stringCondition: %s",
+                      keyPattern, printMessage(stringCondition)))
+              .asRuntimeException();
+        }
+        break;
+      default:
+        throwInvalidArgumentException(
+            String.format(
+                "Invalid operator for key condition: %s: %s",
+                getName(stringCondition), printMessage(stringCondition)));
+    }
+  }
+
   private void validateStringCondition(StringCondition stringCondition) {
     validateNonDefaultPresenceOrThrow(stringCondition, StringCondition.OPERATOR_FIELD_NUMBER);
-    if (stringCondition.getOperator().equals(OPERATOR_MATCHES_IPS)
-        || stringCondition.getOperator().equals(OPERATOR_NOT_MATCHES_IPS)) {
-      switch (stringCondition.getKindCase()) {
-        case VALUE:
-          final String ip = stringCondition.getValue();
-          if (!isValidIpAddressOrSubnet(ip)) {
-            throwInvalidArgumentException(
-                "StringCondition in LabelApplicationRule should have a valid IP address or CIDR");
-          }
-          break;
-        case VALUES:
-          if (stringCondition.getValues().getValuesList().stream()
-              .anyMatch(s -> !isValidIpAddressOrSubnet(s))) {
+    switch (stringCondition.getOperator()) {
+      case OPERATOR_MATCHES_REGEX:
+        validateNonDefaultPresenceOrThrow(stringCondition, StringCondition.VALUE_FIELD_NUMBER);
+        final String pattern = stringCondition.getValue();
+        final Status regexValidationStatus = RegexValidator.validate(pattern);
+        if (!regexValidationStatus.isOk()) {
+          throw regexValidationStatus
+              .withDescription(String.format("Invalid regex : %s", pattern))
+              .asRuntimeException();
+        }
+        break;
+      case OPERATOR_MATCHES_IPS:
+      case OPERATOR_NOT_MATCHES_IPS:
+        switch (stringCondition.getKindCase()) {
+          case VALUE:
+            final String ip = stringCondition.getValue();
+            if (!isValidIpAddressOrSubnet(ip)) {
+              throwInvalidArgumentException(
+                  String.format(
+                      "Invalid IP address or CIDR in StringCondition: %s",
+                      printMessage(stringCondition)));
+            }
+            break;
+          case VALUES:
+            if (stringCondition.getValues().getValuesList().stream()
+                .anyMatch(s -> !isValidIpAddressOrSubnet(s))) {
+              throwInvalidArgumentException(
+                  String.format(
+                      "Invalid list of IP addresses or CIDRs in StringCondition: %s",
+                      printMessage(stringCondition)));
+            }
+            break;
+          default:
             throwInvalidArgumentException(
-                "StringCondition in LabelApplicationRule should have a valid list of IP addresses and CIDRs");
-          }
-          break;
-        default:
-          throwInvalidArgumentException(
-              String.format(
-                  "Unexpected Case in %s:%n %s",
-                  getName(stringCondition), printMessage(stringCondition)));
-      }
+                String.format(
+                    "Unexpected Case in %s:%n %s",
+                    getName(stringCondition), printMessage(stringCondition)));
+        }
     }
   }
 

diff --git a/...g/hypertrace/label/application/rule/config/service/LabelApplicationRuleValidatorTest.java b/...g/hypertrace/label/application/rule/config/service/LabelApplicationRuleValidatorTest.java
@@ -24,10 +24,11 @@ public class LabelApplicationRuleValidatorTest {
   private final LabelApplicationRuleValidator labelApplicationRuleValidator;
   private final StringCondition errorKeyCondition;
   private final StringCondition correctKeyCondition;
-  private final StringCondition correctKeyConditionWithMatchesIPs;
-  private final StringCondition incorrectKeyConditionWithMatchesIPs;
+  private final StringCondition correctKeyConditionMatchesRegex;
   private final StringCondition correctAuthKeyCondition;
   private final StringCondition correctStringValueCondition;
+  private final StringCondition correctRegexStringValueCondition;
+  private final StringCondition incorrectRegexStringValueCondition;
   private final StringCondition correctStringValueConditionWithMatchesIPs;
   private final StringCondition incorrectStringValueConditionWithMatchesIPs;
   private final UnaryCondition errorUnaryValueCondition;
@@ -47,15 +48,10 @@ public LabelApplicationRuleValidatorTest() {
             .setOperator(StringCondition.Operator.OPERATOR_EQUALS)
             .setValue("foo")
             .build();
-    correctKeyConditionWithMatchesIPs =
+    correctKeyConditionMatchesRegex =
         StringCondition.newBuilder()
-            .setOperator(StringCondition.Operator.OPERATOR_MATCHES_IPS)
-            .setValue("1.2.3.4")
-            .build();
-    incorrectKeyConditionWithMatchesIPs =
-        StringCondition.newBuilder()
-            .setOperator(StringCondition.Operator.OPERATOR_MATCHES_IPS)
-            .setValue("4.5.6.7/s")
+            .setOperator(StringCondition.Operator.OPERATOR_MATCHES_REGEX)
+            .setValue("foo.*")
             .build();
     errorUnaryValueCondition =
         UnaryCondition.newBuilder()
@@ -68,6 +64,16 @@ public LabelApplicationRuleValidatorTest() {
             .setOperator(StringCondition.Operator.OPERATOR_EQUALS)
             .setValue("bar")
             .build();
+    correctRegexStringValueCondition =
+        StringCondition.newBuilder()
+            .setOperator(StringCondition.Operator.OPERATOR_MATCHES_REGEX)
+            .setValue(".*bar.*")
+            .build();
+    incorrectRegexStringValueCondition =
+        StringCondition.newBuilder()
+            .setOperator(StringCondition.Operator.OPERATOR_MATCHES_REGEX)
+            .setValue("((?!bar)")
+            .build();
     correctStringValueConditionWithMatchesIPs =
         StringCondition.newBuilder()
             .setOperator(StringCondition.Operator.OPERATOR_MATCHES_IPS)
@@ -179,7 +185,7 @@ void validateOrThrowCreateRuleLeafConditionWithMatchesIPs() {
     // This will check the condition that foo(key) = bar(value)
     LeafCondition errorLeafCondition =
         LeafCondition.newBuilder()
-            .setKeyCondition(correctKeyConditionWithMatchesIPs)
+            .setKeyCondition(correctKeyCondition)
             .setStringCondition(correctStringValueConditionWithMatchesIPs)
             .build();
     Condition matchingCondition =
@@ -195,7 +201,7 @@ void validateOrThrowCreateRuleInvalidLeafConditionWithMatchesIPs() {
     // This will check the condition that foo(key) = bar(value)
     LeafCondition errorLeafCondition =
         LeafCondition.newBuilder()
-            .setKeyCondition(incorrectKeyConditionWithMatchesIPs)
+            .setKeyCondition(correctKeyConditionMatchesRegex)
             .setStringCondition(incorrectStringValueConditionWithMatchesIPs)
             .build();
     Condition matchingCondition =
@@ -319,6 +325,43 @@ void validateDisabledRule() {
         () -> labelApplicationRuleValidator.validateOrThrow(REQUEST_CONTEXT, request));
   }
 
+  @Test
+  void validateOrThrowCreateRuleLeafConditionWithMatchRegex() {
+    // This will check the condition that foo(key) = bar(value)
+    LeafCondition correctLeafCondition =
+        LeafCondition.newBuilder()
+            .setKeyCondition(correctKeyCondition)
+            .setStringCondition(correctRegexStringValueCondition)
+            .build();
+    Condition matchingCondition =
+        Condition.newBuilder().setLeafCondition(correctLeafCondition).build();
+    CreateLabelApplicationRuleRequest request =
+        buildCreateCreateLabelApplicationRuleRequest(
+            "Correct Leaf Rule", matchingCondition, Optional.empty());
+    labelApplicationRuleValidator.validateOrThrow(REQUEST_CONTEXT, request);
+  }
+
+  @Test
+  void validateOrThrowCreateRuleInvalidLeafConditionWithMatchRegex() {
+    // This will check the condition that foo(key) = bar(value)
+    LeafCondition invalidLeafCondition =
+        LeafCondition.newBuilder()
+            .setKeyCondition(correctKeyCondition)
+            .setStringCondition(incorrectRegexStringValueCondition)
+            .build();
+    Condition matchingCondition =
+        Condition.newBuilder().setLeafCondition(invalidLeafCondition).build();
+    CreateLabelApplicationRuleRequest request =
+        buildCreateCreateLabelApplicationRuleRequest(
+            "Incorrect Leaf Rule", matchingCondition, Optional.empty());
+
+    assertThrows(
+        StatusRuntimeException.class,
+        () -> {
+          labelApplicationRuleValidator.validateOrThrow(REQUEST_CONTEXT, request);
+        });
+  }
+
   private CreateLabelApplicationRuleRequest buildCreateCreateLabelApplicationRuleRequest(
       String name, Condition matchingCondition, Optional<String> labelExpression) {
     return buildCreateCreateLabelApplicationRuleRequest(

diff --git a/notification-channel-config-service-impl/gradle.lockfile b/notification-channel-config-service-impl/gradle.lockfile
@@ -21,6 +21,7 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava=comp
 com.google.j2objc:j2objc-annotations:2.8=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java-util:3.24.1=runtimeClasspath
 com.google.protobuf:protobuf-java:3.24.1=compileClasspath,runtimeClasspath
+com.google.re2j:re2j:1.7=runtimeClasspath
 com.typesafe:config:1.4.2=compileClasspath,runtimeClasspath
 commons-io:commons-io:2.7=runtimeClasspath
 io.grpc:grpc-api:1.60.0=compileClasspath,runtimeClasspath

diff --git a/notification-rule-config-service-impl/gradle.lockfile b/notification-rule-config-service-impl/gradle.lockfile
@@ -21,6 +21,7 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava=comp
 com.google.j2objc:j2objc-annotations:2.8=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java-util:3.24.1=runtimeClasspath
 com.google.protobuf:protobuf-java:3.24.1=compileClasspath,runtimeClasspath
+com.google.re2j:re2j:1.7=runtimeClasspath
 com.typesafe:config:1.4.2=compileClasspath,runtimeClasspath
 commons-io:commons-io:2.7=runtimeClasspath
 io.grpc:grpc-api:1.60.0=compileClasspath,runtimeClasspath

diff --git a/validation-utils/build.gradle.kts b/validation-utils/build.gradle.kts
@@ -7,4 +7,5 @@ dependencies {
   api(commonLibs.grpc.api)
   implementation(commonLibs.protobuf.javautil)
   implementation(localLibs.seancfoley.ipaddress)
+  implementation(commonLibs.google.re2j)
 }
diff --git a/validation-utils/gradle.lockfile b/validation-utils/gradle.lockfile
@@ -20,6 +20,7 @@ com.google.guava:listenablefuture:9999.0-empty-to-avoid-conflict-with-guava=comp
 com.google.j2objc:j2objc-annotations:2.8=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java-util:3.24.1=compileClasspath,runtimeClasspath
 com.google.protobuf:protobuf-java:3.24.1=compileClasspath,runtimeClasspath
+com.google.re2j:re2j:1.7=compileClasspath,runtimeClasspath
 io.grpc:grpc-api:1.60.0=compileClasspath,runtimeClasspath
 io.grpc:grpc-bom:1.60.0=compileClasspath,runtimeClasspath
 io.grpc:grpc-context:1.60.0=runtimeClasspath

diff --git a/validation-utils/src/main/java/org/hypertrace/config/validation/RegexValidator.java b/validation-utils/src/main/java/org/hypertrace/config/validation/RegexValidator.java
@@ -0,0 +1,19 @@
+package org.hypertrace.config.validation;
+
+import com.google.re2j.Pattern;
+import com.google.re2j.PatternSyntaxException;
+import io.grpc.Status;
+
+public class RegexValidator {
+  public static Status validate(String regexPattern) {
+    // compiling an invalid regex throws PatternSyntaxException
+    try {
+      Pattern.compile(regexPattern);
+    } catch (PatternSyntaxException ex) {
+      return Status.INVALID_ARGUMENT
+          .withCause(ex)
+          .withDescription("Invalid Regex pattern: " + regexPattern);
+    }
+    return Status.OK;
+  }
+}