diff --git a/modules/api/src/main/scala/vinyldns/api/domain/DomainValidations.scala b/modules/api/src/main/scala/vinyldns/api/domain/DomainValidations.scala index 5a12eb74b..091b79bcd 100644 --- a/modules/api/src/main/scala/vinyldns/api/domain/DomainValidations.scala +++ b/modules/api/src/main/scala/vinyldns/api/domain/DomainValidations.scala @@ -27,6 +27,7 @@ import scala.util.matching.Regex Object to house common domain validations */ object DomainValidations { + val validReverseZoneFQDNRegex: Regex = """^(?:([0-9a-zA-Z\-\/_]{1,63}|[0-9a-zA-Z\-\/_]{1}[0-9a-zA-Z\-\/_]{0,61}[0-9a-zA-Z\-\/_]{1}|[*.]{2}[0-9a-zA-Z\-\/_]{0,60}[0-9a-zA-Z\-\/_]{1})\.)*$""".r val validForwardZoneFQDNRegex: Regex = @@ -61,13 +62,20 @@ object DomainValidations { val MX_PREFERENCE_MIN_VALUE: Int = 0 val MX_PREFERENCE_MAX_VALUE: Int = 65535 + // Cname check - Cname should not be IP address + def validateCname(name: Fqdn, isReverse: Boolean): ValidatedNel[DomainValidationError, Fqdn] = + validateIpv4Address(name.fqdn.dropRight(1)).isValid match { + case true => InvalidIPv4CName(name.toString).invalidNel + case false => validateIsReverseCname(name, isReverse) + } + def validateHostName(name: Fqdn): ValidatedNel[DomainValidationError, Fqdn] = validateHostName(name.fqdn).map(_ => name) - def validateCname(name: Fqdn, isReverse: Boolean): ValidatedNel[DomainValidationError, Fqdn] = - validateCname(name.fqdn, isReverse).map(_ => name) + def validateIsReverseCname(name: Fqdn, isReverse: Boolean): ValidatedNel[DomainValidationError, Fqdn] = + validateIsReverseCname(name.fqdn, isReverse).map(_ => name) - def validateCname(name: String, isReverse: Boolean): ValidatedNel[DomainValidationError, String] = { + def validateIsReverseCname(name: String, isReverse: Boolean): ValidatedNel[DomainValidationError, String] = { isReverse match { case true => val checkRegex = validReverseZoneFQDNRegex diff --git a/modules/api/src/test/functional/tests/batch/create_batch_change_test.py b/modules/api/src/test/functional/tests/batch/create_batch_change_test.py index b90a2c418..86fbadaef 100644 --- a/modules/api/src/test/functional/tests/batch/create_batch_change_test.py +++ b/modules/api/src/test/functional/tests/batch/create_batch_change_test.py @@ -1938,7 +1938,8 @@ def test_cname_recordtype_add_checks(shared_zone_test_context): get_change_CNAME_json(existing_forward_fqdn), get_change_CNAME_json(existing_cname_fqdn), get_change_CNAME_json(f"0.{ip4_zone_name}", cname="duplicate.in.db."), - get_change_CNAME_json(f"user-add-unauthorized.{dummy_zone_name}") + get_change_CNAME_json(f"user-add-unauthorized.{dummy_zone_name}"), + get_change_CNAME_json(f"invalid-ipv4-{parent_zone_name}", cname="1.2.3.4") ] } @@ -2014,6 +2015,9 @@ def test_cname_recordtype_add_checks(shared_zone_test_context): assert_failed_change_in_error_response(response[16], input_name=f"user-add-unauthorized.{dummy_zone_name}", record_type="CNAME", record_data="test.com.", error_messages=[f"User \"ok\" is not authorized. Contact zone owner group: {dummy_group_name} at test@test.com to make DNS changes."]) + assert_failed_change_in_error_response(response[17], input_name=f"invalid-ipv4-{parent_zone_name}", record_type="CNAME", record_data="1.2.3.4.", + error_messages=[f'Invalid Cname: "Fqdn(1.2.3.4.)", Valid CNAME record data should not be an IP address']) + finally: clear_recordset_list(to_delete, client) diff --git a/modules/api/src/test/scala/vinyldns/api/domain/DomainValidationsSpec.scala b/modules/api/src/test/scala/vinyldns/api/domain/DomainValidationsSpec.scala index 3101f9571..68dc9535e 100644 --- a/modules/api/src/test/scala/vinyldns/api/domain/DomainValidationsSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/domain/DomainValidationsSpec.scala @@ -22,7 +22,7 @@ import org.scalatestplus.scalacheck.ScalaCheckDrivenPropertyChecks import org.scalatest.propspec.AnyPropSpec import org.scalatest.matchers.should.Matchers import vinyldns.api.ValidationTestImprovements._ -import vinyldns.core.domain.{InvalidDomainName, InvalidCname, InvalidLength} +import vinyldns.core.domain.{InvalidDomainName, Fqdn, InvalidCname, InvalidLength} class DomainValidationsSpec extends AnyPropSpec @@ -77,6 +77,54 @@ class DomainValidationsSpec validateHostName("asterisk.domain*.name.") shouldBe invalid } + property("Shortest fqdn name should be valid") { + val fqdn = Fqdn("a.") + validateCname(fqdn, false) shouldBe valid + } + + property("Ip address in cname should be invalid") { + val fqdn = Fqdn("1.2.3.4") + validateCname(fqdn, false) shouldBe invalid + } + + property("Longest fqdn name should be valid") { + val fqdn = Fqdn(("a" * 50 + ".") * 5) + validateCname(fqdn, false) shouldBe valid + } + + property("fqdn name should pass property-based testing") { + forAll(domainGenerator) { domain: String => + val domains= Fqdn(domain) + whenever(validateHostName(domains).isValid) { + domains.fqdn.length should be > 0 + domains.fqdn.length should be < 256 + (domains.fqdn should fullyMatch).regex(validFQDNRegex) + domains.fqdn should endWith(".") + } + } + } + + property("fqdn names beginning with invalid characters should fail with InvalidCname") { + validateCname(Fqdn("/slash.domain.name."), false).failWith[InvalidCname] + validateCname(Fqdn("-hyphen.domain.name."), false).failWith[InvalidCname] + } + + property("fqdn names with underscores should pass property-based testing") { + validateCname(Fqdn("_underscore.domain.name."), false).isValid + validateCname(Fqdn("under_score.domain.name."), false).isValid + validateCname(Fqdn("underscore._domain.name."), false).isValid + } + + // For wildcard records. '*' can only be in the beginning followed by '.' and domain name + property("fqdn names beginning with asterisk should pass property-based testing") { + validateCname(Fqdn("*.domain.name."), false) shouldBe valid + validateCname(Fqdn("aste*risk.domain.name."),false) shouldBe invalid + validateCname(Fqdn("*asterisk.domain.name."),false) shouldBe invalid + validateCname(Fqdn("asterisk*.domain.name."),false) shouldBe invalid + validateCname(Fqdn("asterisk.*domain.name."),false)shouldBe invalid + validateCname(Fqdn("asterisk.domain*.name."),false) shouldBe invalid + } + property("Valid Ipv4 addresses should pass property-based testing") { forAll(validIpv4Gen) { validIp: String => val res = validateIpv4Address(validIp) @@ -112,50 +160,50 @@ class DomainValidationsSpec } property("Shortest cname should be valid") { - validateCname("a.",true) shouldBe valid - validateCname("a.",false) shouldBe valid + validateIsReverseCname("a.",true) shouldBe valid + validateIsReverseCname("a.",false) shouldBe valid } property("Longest cname should be valid") { val name = ("a" * 50 + ".") * 5 - validateCname(name,true) shouldBe valid - validateCname(name,false) shouldBe valid + validateIsReverseCname(name,true) shouldBe valid + validateIsReverseCname(name,false) shouldBe valid } property("Cnames with underscores should pass property-based testing") { - validateCname("_underscore.domain.name.",true).isValid - validateCname("under_score.domain.name.",true).isValid - validateCname("underscore._domain.name.",true).isValid - validateCname("_underscore.domain.name.",false).isValid - validateCname("under_score.domain.name.",false).isValid - validateCname("underscore._domain.name.",false).isValid + validateIsReverseCname("_underscore.domain.name.",true).isValid + validateIsReverseCname("under_score.domain.name.",true).isValid + validateIsReverseCname("underscore._domain.name.",true).isValid + validateIsReverseCname("_underscore.domain.name.",false).isValid + validateIsReverseCname("under_score.domain.name.",false).isValid + validateIsReverseCname("underscore._domain.name.",false).isValid } // For wildcard records. '*' can only be in the beginning followed by '.' and domain name property("Cnames beginning with asterisk should pass property-based testing") { - validateCname("*.domain.name.",true) shouldBe valid - validateCname("aste*risk.domain.name.",true) shouldBe invalid - validateCname("*asterisk.domain.name.",true) shouldBe invalid - validateCname("asterisk*.domain.name.",true) shouldBe invalid - validateCname("asterisk.*domain.name.",true) shouldBe invalid - validateCname("asterisk.domain*.name.",true) shouldBe invalid - validateCname("*.domain.name.",false) shouldBe valid - validateCname("aste*risk.domain.name.",false) shouldBe invalid - validateCname("*asterisk.domain.name.",false) shouldBe invalid - validateCname("asterisk*.domain.name.",false) shouldBe invalid - validateCname("asterisk.*domain.name.",false) shouldBe invalid - validateCname("asterisk.domain*.name.",false) shouldBe invalid + validateIsReverseCname("*.domain.name.",true) shouldBe valid + validateIsReverseCname("aste*risk.domain.name.",true) shouldBe invalid + validateIsReverseCname("*asterisk.domain.name.",true) shouldBe invalid + validateIsReverseCname("asterisk*.domain.name.",true) shouldBe invalid + validateIsReverseCname("asterisk.*domain.name.",true) shouldBe invalid + validateIsReverseCname("asterisk.domain*.name.",true) shouldBe invalid + validateIsReverseCname("*.domain.name.",false) shouldBe valid + validateIsReverseCname("aste*risk.domain.name.",false) shouldBe invalid + validateIsReverseCname("*asterisk.domain.name.",false) shouldBe invalid + validateIsReverseCname("asterisk*.domain.name.",false) shouldBe invalid + validateIsReverseCname("asterisk.*domain.name.",false) shouldBe invalid + validateIsReverseCname("asterisk.domain*.name.",false) shouldBe invalid } property("Cname names with forward slash should pass with reverse zone") { - validateCname("/slash.cname.name.",true).isValid - validateCname("slash./cname.name.",true).isValid - validateCname("slash.cname./name.",true).isValid + validateIsReverseCname("/slash.cname.name.",true).isValid + validateIsReverseCname("slash./cname.name.",true).isValid + validateIsReverseCname("slash.cname./name.",true).isValid } property("Cname names with forward slash should fail with forward zone") { - validateCname("/slash.cname.name.",false).failWith[InvalidCname] - validateCname("slash./cname.name.",false).failWith[InvalidCname] - validateCname("slash.cname./name.",false).failWith[InvalidCname] + validateIsReverseCname("/slash.cname.name.",false).failWith[InvalidCname] + validateIsReverseCname("slash./cname.name.",false).failWith[InvalidCname] + validateIsReverseCname("slash.cname./name.",false).failWith[InvalidCname] } } diff --git a/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala b/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala index 29b21d62b..feb55f916 100644 --- a/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/domain/batch/BatchChangeValidationsSpec.scala @@ -717,6 +717,21 @@ class BatchChangeValidationsSpec result should haveInvalid[DomainValidationError](InvalidCname(s"$invalidCNAMERecordData.",false)) } + property("""validateAddChangeInput: should fail with Invalid CNAME + |if validateRecordData fails for IPv4 Address in CNAME record data""".stripMargin) { + val invalidCNAMERecordData = "1.2.3.4" + val change = + AddChangeInput( + "test.comcast.com.", + RecordType.CNAME, + ttl, + CNAMEData(Fqdn(invalidCNAMERecordData)) + ) + val result = validateAddChangeInput(change, false) + + result should haveInvalid[DomainValidationError](InvalidIPv4CName(s"Fqdn($invalidCNAMERecordData.)")) + } + property("""validateAddChangeInput: should fail with InvalidLength |if validateRecordData fails for invalid CNAME record data""".stripMargin) { val invalidCNAMERecordData = "s" * 256 diff --git a/modules/api/src/test/scala/vinyldns/api/domain/record/RecordSetServiceSpec.scala b/modules/api/src/test/scala/vinyldns/api/domain/record/RecordSetServiceSpec.scala index 3ee6f3e86..1f68ce793 100644 --- a/modules/api/src/test/scala/vinyldns/api/domain/record/RecordSetServiceSpec.scala +++ b/modules/api/src/test/scala/vinyldns/api/domain/record/RecordSetServiceSpec.scala @@ -1331,7 +1331,7 @@ class RecordSetServiceSpec doReturn(IO.pure(ListUsersResults(Seq(), None))) .when(mockUserRepo) .getUsers(Set.empty, None, None) - + val result = underTest.updateRecordSet(newRecord, auth).map(_.asInstanceOf[RecordSetChange]).value.unsafeRunSync().toOption.get diff --git a/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala b/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala index a6e48b179..cc44f6866 100644 --- a/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala +++ b/modules/core/src/main/scala/vinyldns/core/domain/DomainValidationErrors.scala @@ -52,6 +52,11 @@ final case class InvalidDomainName(param: String) extends DomainValidationError "joined by dots, and terminated with a dot." } +final case class InvalidIPv4CName(param: String) extends DomainValidationError { + def message: String = + s"""Invalid Cname: "$param", Valid CNAME record data should not be an IP address""" +} + final case class InvalidCname(param: String, isReverseZone: Boolean) extends DomainValidationError { def message: String = isReverseZone match { diff --git a/modules/core/src/main/scala/vinyldns/core/domain/SingleChangeError.scala b/modules/core/src/main/scala/vinyldns/core/domain/SingleChangeError.scala index 774be6a8b..5e384cd4a 100644 --- a/modules/core/src/main/scala/vinyldns/core/domain/SingleChangeError.scala +++ b/modules/core/src/main/scala/vinyldns/core/domain/SingleChangeError.scala @@ -36,7 +36,7 @@ object DomainValidationErrorType extends Enumeration { CnameIsNotUniqueError, UserIsNotAuthorized, UserIsNotAuthorizedError, RecordNameNotUniqueInBatch, RecordInReverseZoneError, HighValueDomainError, MissingOwnerGroupId, ExistingMultiRecordError, NewMultiRecordError, CnameAtZoneApexError, RecordRequiresManualReview, UnsupportedOperation, - DeleteRecordDataDoesNotExist = Value + DeleteRecordDataDoesNotExist, InvalidIPv4CName = Value // $COVERAGE-OFF$ def from(error: DomainValidationError): DomainValidationErrorType = @@ -74,6 +74,7 @@ object DomainValidationErrorType extends Enumeration { case _: RecordRequiresManualReview => RecordRequiresManualReview case _: UnsupportedOperation => UnsupportedOperation case _: DeleteRecordDataDoesNotExist => DeleteRecordDataDoesNotExist + case _: InvalidIPv4CName => InvalidIPv4CName } // $COVERAGE-ON$ }