Add name validation to task definitions and workflow definitions

common/src/main/java/com/netflix/conductor/common/constraints/ValidNameConstraint.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright 2020 Conductor Authors.
+ * <p>
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package com.netflix.conductor.common.constraints;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+import org.springframework.beans.factory.annotation.Value;
+
+import jakarta.validation.Constraint;
+import jakarta.validation.ConstraintValidator;
+import jakarta.validation.ConstraintValidatorContext;
+import jakarta.validation.Payload;
+
+import static java.lang.annotation.ElementType.FIELD;
+
+/**
+ * This constraint class validates following things.
+ *
+ * <ul>
+ *   <li>1. Name is valid or not
+ * </ul>
+ */
+@Documented
+@Constraint(validatedBy = ValidNameConstraint.NameValidator.class)
+@Target({FIELD})
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ValidNameConstraint {
+
+    String message() default "";
+
+    Class<?>[] groups() default {};
+
+    Class<? extends Payload>[] payload() default {};
+
+    class NameValidator implements ConstraintValidator<ValidNameConstraint, String> {
+
+        private static final String NAME_PATTERN = "^[A-Za-z0-9_<>{}#\\s-]+$";
+        public static final String INVALID_NAME_MESSAGE =
+                "Allowed characters are alphanumeric, underscores, spaces, hyphens, and special characters like <, >, {, }, #";
+
+        @Value("${conductor.app.workflow.name-validation.enabled}")
+        private boolean nameValidationEnabled;
+
+        @Override
+        public void initialize(ValidNameConstraint constraintAnnotation) {}
+
+        @Override
+        public boolean isValid(String name, ConstraintValidatorContext context) {
+            boolean valid = name == null || !nameValidationEnabled || name.matches(NAME_PATTERN);
+            if (!valid) {
+                context.disableDefaultConstraintViolation();
+                context.buildConstraintViolationWithTemplate(
+                                "Invalid name '" + name + "'. " + INVALID_NAME_MESSAGE)
+                        .addConstraintViolation();
+            }
+            return valid;
+        }
+    }
+}

common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java

@@ -17,9 +17,9 @@
 import com.netflix.conductor.annotations.protogen.ProtoEnum;
 import com.netflix.conductor.annotations.protogen.ProtoField;
 import com.netflix.conductor.annotations.protogen.ProtoMessage;
-import com.netflix.conductor.common.constraints.NoSemiColonConstraint;
 import com.netflix.conductor.common.constraints.OwnerEmailMandatoryConstraint;
 import com.netflix.conductor.common.constraints.TaskReferenceNameUniqueConstraint;
+import com.netflix.conductor.common.constraints.ValidNameConstraint;
 import com.netflix.conductor.common.metadata.Auditable;
 import com.netflix.conductor.common.metadata.SchemaDef;
 import com.netflix.conductor.common.metadata.tasks.TaskType;
@@ -39,8 +39,7 @@ public enum TimeoutPolicy {
 
     @NotEmpty(message = "WorkflowDef name cannot be null or empty")
     @ProtoField(id = 1)
-    @NoSemiColonConstraint(
-            message = "Workflow name cannot contain the following set of characters: ':'")
+    @ValidNameConstraint
     private String name;
 
     @ProtoField(id = 2)

common/src/test/java/com/netflix/conductor/common/constraints/NameValidatorTest.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright 2024 Conductor Authors.
+ * <p>
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package com.netflix.conductor.common.constraints;
+
+import org.junit.Test;
+import org.springframework.test.util.ReflectionTestUtils;
+
+import jakarta.validation.ConstraintValidatorContext;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class NameValidatorTest {
+    @Test
+    public void nameWithAllowedCharactersIsValid() {
+        ValidNameConstraint.NameValidator nameValidator = new ValidNameConstraint.NameValidator();
+        assertTrue(nameValidator.isValid("workflowDef", null));
+    }
+
+    @Test
+    public void nonAllowedCharactersInNameIsInvalid() {
+        ValidNameConstraint.NameValidator nameValidator = new ValidNameConstraint.NameValidator();
+        ConstraintValidatorContext context = mock(ConstraintValidatorContext.class);
+        ConstraintValidatorContext.ConstraintViolationBuilder builder =
+                mock(ConstraintValidatorContext.ConstraintViolationBuilder.class);
+        when(context.buildConstraintViolationWithTemplate(anyString())).thenReturn(builder);
+
+        ReflectionTestUtils.setField(nameValidator, "nameValidationEnabled", true);
+
+        assertFalse(nameValidator.isValid("workflowDef@", context));
+    }
+
+    // Null should be tested by @NotEmpty or @NotNull
+    @Test
+    public void nullIsValid() {
+        ValidNameConstraint.NameValidator nameValidator = new ValidNameConstraint.NameValidator();
+        assertTrue(nameValidator.isValid(null, null));
+    }
+}

common/src/test/java/com/netflix/conductor/common/workflow/WorkflowDefValidatorTest.java

@@ -20,6 +20,7 @@
 
 import org.junit.Before;
 import org.junit.Test;
+import org.springframework.test.context.TestPropertySource;
 
 import com.netflix.conductor.common.metadata.tasks.TaskType;
 import com.netflix.conductor.common.metadata.workflow.WorkflowDef;
@@ -33,6 +34,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
+@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
 public class WorkflowDefValidatorTest {
 
     @Before

common/src/test/resources/application.properties

@@ -0,0 +1 @@
+conductor.app.workflow.name-validation.enabled=true

core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java

@@ -21,6 +21,7 @@
 import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
 import org.springframework.boot.test.context.TestConfiguration;
 import org.springframework.context.annotation.Bean;
+import org.springframework.test.context.TestPropertySource;
 import org.springframework.test.context.junit4.SpringRunner;
 
 import com.netflix.conductor.common.metadata.events.EventHandler;
@@ -50,6 +51,7 @@
 
 @SuppressWarnings("SpringJavaAutowiredMembersInspection")
 @RunWith(SpringRunner.class)
+@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
 @EnableAutoConfiguration
 public class MetadataServiceTest {
 
@@ -64,6 +66,7 @@ public MetadataDAO metadataDAO() {
         public ConductorProperties properties() {
             ConductorProperties properties = mock(ConductorProperties.class);
             when(properties.isOwnerEmailMandatory()).thenReturn(true);
+
             return properties;
         }
 
@@ -415,7 +418,7 @@ public void testRegisterWorkflowDefInvalidName() {
             assertTrue(messages.contains("WorkflowTask list cannot be empty"));
             assertTrue(
                     messages.contains(
-                            "Workflow name cannot contain the following set of characters: ':'"));
+                            "Invalid name 'invalid:name'. Allowed characters are alphanumeric, underscores, spaces, hyphens, and special characters like <, >, {, }, #"));
             throw ex;
         }
         fail("metadataService.registerWorkflowDef did not throw ConstraintViolationException !");
@@ -434,7 +437,7 @@ public void testValidateWorkflowDefInvalidName() {
             assertTrue(messages.contains("WorkflowTask list cannot be empty"));
             assertTrue(
                     messages.contains(
-                            "Workflow name cannot contain the following set of characters: ':'"));
+                            "Invalid name 'invalid:name'. Allowed characters are alphanumeric, underscores, spaces, hyphens, and special characters like <, >, {, }, #"));
             throw ex;
         }
         fail("metadataService.validateWorkflowDef did not throw ConstraintViolationException !");

kafka/src/test/groovy/com/netflix/conductor/test/integration/KafkaPublishTaskSpec.groovy

@@ -16,15 +16,15 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.conductor.common.metadata.tasks.TaskDef
 import com.netflix.conductor.common.metadata.tasks.TaskResult
 import com.netflix.conductor.common.metadata.tasks.TaskType
-import com.netflix.conductor.common.metadata.workflow.StartWorkflowRequest
 import com.netflix.conductor.common.metadata.workflow.WorkflowDef
 import com.netflix.conductor.common.metadata.workflow.WorkflowTask
 import com.netflix.conductor.common.run.Workflow
-import com.netflix.conductor.core.execution.StartWorkflowInput
 import com.netflix.conductor.test.base.AbstractSpecification
 import org.springframework.beans.factory.annotation.Autowired
+import org.springframework.test.context.TestPropertySource
 import spock.lang.Shared
 
+@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
 class KafkaPublishTaskSpec extends AbstractSpecification {
 
     @Autowired

mysql-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/mysql/MySQLGrpcEndToEndTest.java

@@ -35,7 +35,8 @@
             "spring.datasource.password=root",
             "spring.datasource.hikari.maximum-pool-size=8",
             "spring.datasource.hikari.minimum-idle=300000",
-            "conductor.elasticsearch.version=7"
+            "conductor.elasticsearch.version=7",
+            "conductor.app.workflow.name-validation.enabled=true"
         })
 public class MySQLGrpcEndToEndTest extends AbstractGrpcEndToEndTest {
 

postgres-persistence/src/test/java/com/netflix/conductor/postgres/dao/PostgresLockDAOTest.java

@@ -48,7 +48,8 @@
 @TestPropertySource(
         properties = {
             "conductor.workflow-execution-lock.type=postgres",
-            "spring.flyway.clean-disabled=false"
+            "spring.flyway.clean-disabled=false",
+            "conductor.app.workflow.name-validation.enabled=true"
         })
 @SpringBootTest
 public class PostgresLockDAOTest {

postgres-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/postgres/PostgresGrpcEndToEndTest.java

@@ -39,7 +39,8 @@
             "spring.datasource.password=postgres",
             "spring.datasource.hikari.maximum-pool-size=8",
             "spring.datasource.hikari.minimum-idle=300000",
-            "spring.flyway.clean-disabled=true"
+            "spring.flyway.clean-disabled=true",
+            "conductor.app.workflow.name-validation.enabled=true"
         })
 public class PostgresGrpcEndToEndTest extends AbstractGrpcEndToEndTest {
 

server/src/main/resources/application.properties

@@ -72,6 +72,8 @@ conductor.default-event-queue.type=sqs
 #disable locking during workflow execution
 conductor.app.workflow-execution-lock-enabled=false
 conductor.workflow-execution-lock.type=noop_lock
+# enable name validation on workflow/task definitions
+conductor.app.workflow.name-validation.enabled=false
 
 #Redis cluster settings for locking module
 # conductor.redis-lock.serverType=single

test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/QueueResiliencySpec.groovy

@@ -14,6 +14,7 @@ package com.netflix.conductor.test.resiliency
 
 import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.http.HttpStatus
+import org.springframework.test.context.TestPropertySource
 
 import com.netflix.conductor.common.metadata.tasks.Task
 import com.netflix.conductor.common.metadata.tasks.TaskResult
@@ -36,6 +37,7 @@ import com.netflix.conductor.test.base.AbstractResiliencySpecification
  * 2. Succeeds
  * 3. Doesn't involve QueueDAO
  */
+@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
 class QueueResiliencySpec extends AbstractResiliencySpecification {
 
     @Autowired

test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/TaskResiliencySpec.groovy

@@ -13,6 +13,7 @@
 package com.netflix.conductor.test.resiliency
 
 import org.springframework.beans.factory.annotation.Autowired
+import org.springframework.test.context.TestPropertySource
 
 import com.netflix.conductor.common.metadata.tasks.Task
 import com.netflix.conductor.common.run.Workflow
@@ -22,7 +23,7 @@ import com.netflix.conductor.test.base.AbstractResiliencySpecification
 import spock.lang.Shared
 
 import static com.netflix.conductor.test.util.WorkflowTestUtil.verifyPolledAndAcknowledgedTask
-
+@TestPropertySource(properties = "conductor.app.workflow.name-validation.enabled=true")
 class TaskResiliencySpec extends AbstractResiliencySpecification {
 
     @Autowired

test-harness/src/test/resources/application-integrationtest.properties

@@ -35,6 +35,7 @@ conductor.workflow-reconciler.enabled=true
 conductor.workflow-repair-service.enabled=false
 
 conductor.app.workflow-execution-lock-enabled=false
+conductor.app.workflow.name-validation.enabled=true
 
 conductor.app.workflow-input-payload-size-threshold=10KB
 conductor.app.max-workflow-input-payload-size-threshold=10240KB

workflow-event-listener/src/test/java/com/netflix/conductor/test/listener/WorkflowStatusPublisherIntegrationTest.java

@@ -58,7 +58,8 @@
             "conductor.workflow-status-listener.type=queue_publisher",
             "conductor.workflow-status-listener.queue-publisher.successQueue=dummy",
             "conductor.workflow-status-listener.queue-publisher.failureQueue=dummy",
-            "conductor.workflow-status-listener.queue-publisher.finalizeQueue=final"
+            "conductor.workflow-status-listener.queue-publisher.finalizeQueue=final",
+            "conductor.app.workflow.name-validation.enabled=true"
         })
 @TestPropertySource(locations = "classpath:application-integrationtest.properties")
 public class WorkflowStatusPublisherIntegrationTest {