diff --git a/common/src/main/java/com/netflix/conductor/common/constraints/ValidNameConstraint.java b/common/src/main/java/com/netflix/conductor/common/constraints/ValidNameConstraint.java new file mode 100644 index 000000000..41af1415a --- /dev/null +++ b/common/src/main/java/com/netflix/conductor/common/constraints/ValidNameConstraint.java @@ -0,0 +1,72 @@ +/* + * Copyright 2020 Conductor Authors. + *

+ * 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 + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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. + * + *

+ */ +@Documented +@Constraint(validatedBy = ValidNameConstraint.NameValidator.class) +@Target({FIELD}) +@Retention(RetentionPolicy.RUNTIME) +public @interface ValidNameConstraint { + + String message() default ""; + + Class[] groups() default {}; + + Class[] payload() default {}; + + class NameValidator implements ConstraintValidator { + + 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; + } + } +} diff --git a/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java b/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java index 2569294b8..e51e8ca17 100644 --- a/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java +++ b/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) diff --git a/common/src/test/java/com/netflix/conductor/common/constraints/NameValidatorTest.java b/common/src/test/java/com/netflix/conductor/common/constraints/NameValidatorTest.java new file mode 100644 index 000000000..2fc196864 --- /dev/null +++ b/common/src/test/java/com/netflix/conductor/common/constraints/NameValidatorTest.java @@ -0,0 +1,52 @@ +/* + * Copyright 2024 Conductor Authors. + *

+ * 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 + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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)); + } +} diff --git a/common/src/test/java/com/netflix/conductor/common/workflow/WorkflowDefValidatorTest.java b/common/src/test/java/com/netflix/conductor/common/workflow/WorkflowDefValidatorTest.java index 132e33d99..c2b36e688 100644 --- a/common/src/test/java/com/netflix/conductor/common/workflow/WorkflowDefValidatorTest.java +++ b/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 diff --git a/common/src/test/resources/application.properties b/common/src/test/resources/application.properties new file mode 100644 index 000000000..7c95d0a4d --- /dev/null +++ b/common/src/test/resources/application.properties @@ -0,0 +1 @@ +conductor.app.workflow.name-validation.enabled=true diff --git a/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java b/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java index 3f4c028c1..37ed10450 100644 --- a/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java +++ b/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 !"); diff --git a/kafka/src/test/groovy/com/netflix/conductor/test/integration/KafkaPublishTaskSpec.groovy b/kafka/src/test/groovy/com/netflix/conductor/test/integration/KafkaPublishTaskSpec.groovy index 8087dd9d4..671a6f212 100644 --- a/kafka/src/test/groovy/com/netflix/conductor/test/integration/KafkaPublishTaskSpec.groovy +++ b/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 diff --git a/mysql-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/mysql/MySQLGrpcEndToEndTest.java b/mysql-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/mysql/MySQLGrpcEndToEndTest.java index 680623f43..1cc70310a 100644 --- a/mysql-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/mysql/MySQLGrpcEndToEndTest.java +++ b/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 { diff --git a/postgres-persistence/src/test/java/com/netflix/conductor/postgres/dao/PostgresLockDAOTest.java b/postgres-persistence/src/test/java/com/netflix/conductor/postgres/dao/PostgresLockDAOTest.java index 695f15f10..a6fb4b8a3 100644 --- a/postgres-persistence/src/test/java/com/netflix/conductor/postgres/dao/PostgresLockDAOTest.java +++ b/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 { diff --git a/postgres-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/postgres/PostgresGrpcEndToEndTest.java b/postgres-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/postgres/PostgresGrpcEndToEndTest.java index 00651d34f..9b00cbbcc 100644 --- a/postgres-persistence/src/test/java/com/netflix/conductor/test/integration/grpc/postgres/PostgresGrpcEndToEndTest.java +++ b/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 { diff --git a/server/src/main/resources/application.properties b/server/src/main/resources/application.properties index 4fcb33594..e342c390e 100644 --- a/server/src/main/resources/application.properties +++ b/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 diff --git a/test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/QueueResiliencySpec.groovy b/test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/QueueResiliencySpec.groovy index 0149352bd..2b82a7867 100644 --- a/test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/QueueResiliencySpec.groovy +++ b/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 diff --git a/test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/TaskResiliencySpec.groovy b/test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/TaskResiliencySpec.groovy index 4695d6587..1f6d311e4 100644 --- a/test-harness/src/test/groovy/com/netflix/conductor/test/resiliency/TaskResiliencySpec.groovy +++ b/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 diff --git a/test-harness/src/test/resources/application-integrationtest.properties b/test-harness/src/test/resources/application-integrationtest.properties index b209c8054..e6390edce 100644 --- a/test-harness/src/test/resources/application-integrationtest.properties +++ b/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 diff --git a/workflow-event-listener/src/test/java/com/netflix/conductor/test/listener/WorkflowStatusPublisherIntegrationTest.java b/workflow-event-listener/src/test/java/com/netflix/conductor/test/listener/WorkflowStatusPublisherIntegrationTest.java index 33d909a84..38e5f737b 100644 --- a/workflow-event-listener/src/test/java/com/netflix/conductor/test/listener/WorkflowStatusPublisherIntegrationTest.java +++ b/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 {