Validate email addresses via InternetAddress instead of a hand-rolled regex

build.sbt

@@ -4,3 +4,5 @@ version := "1.14.0"
 scalaVersion := "2.13.18"
 gitbucketVersion := "4.47.1"
 scalacOptions += "-deprecation"
+
+libraryDependencies += "org.scalatest" %% "scalatest" % "3.2.19" % "test"

src/main/scala/fr/brouillard/gitbucket/announce/controller/AnnounceController.scala

@@ -64,17 +64,13 @@ trait AnnounceControllerBase extends ControllerBase {
           t.getCause match {
             case ex: SendFailedException => {
               logger.error("found invalid email address while sending notification", ex)
-              if (ex.getInvalidAddresses() != null) {
-                for (ia <- ex.getInvalidAddresses()) {
-                  logger.error("invalid email address: {}", ia.toString())
-                }
-              }
-              if (ex.getValidUnsentAddresses() != null) {
-                for (ua <- ex.getValidUnsentAddresses()) {
-                  logger.error("email not sent to: {}", ua.toString())
-                }
-              }
-              flash.update("info", "Announce has been sent.")
+              val invalidAddresses = Option(ex.getInvalidAddresses()).map(_.toList).getOrElse(Nil)
+              val unsentAddresses = Option(ex.getValidUnsentAddresses()).map(_.toList).getOrElse(Nil)
+              invalidAddresses.foreach(ia => logger.error("invalid email address: {}", ia.toString()))
+              unsentAddresses.foreach(ua => logger.error("email not sent to: {}", ua.toString()))
+              flash.update("info",
+                s"Announce has been sent, but ${invalidAddresses.size} address(es) were invalid and " +
+                  s"${unsentAddresses.size} address(es) were not sent. See server log for details.")
             }
             case _ => {
               logger.error("failure sending email", t)

src/main/scala/fr/brouillard/gitbucket/announce/service/AnnounceService.scala

@@ -1,18 +1,29 @@
 package fr.brouillard.gitbucket.announce.service
 
+import javax.mail.internet.{AddressException, InternetAddress}
+
 import gitbucket.core.model.Profile.profile.blockingApi._
 import gitbucket.core.model.Account
 import gitbucket.core.model.Profile.{Accounts, GroupMembers}
 import gitbucket.core.service.AccountService
+import org.slf4j.LoggerFactory
 
 object EmailAddress {
-  private val EmailRegex = """\b[a-zA-Z0-9.!#$%&¡¯*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*\b""".r
-  def isValid(email: String): Boolean = EmailRegex.pattern.matcher(email.toUpperCase).matches()
+  // Uses InternetAddress, already used in GitBucket's mail-sending path (commons-email -> javax.mail).
+  def isValid(email: String): Boolean =
+    try {
+      new InternetAddress(email).validate()
+      true
+    } catch {
+      case _: AddressException => false
+    }
 }
 
 trait AnnounceService {
   self: AccountService =>
 
+  private val logger = LoggerFactory.getLogger(classOf[AnnounceService])
+
   def getAccountByGroupName(groupName: String)(implicit s: Session): List[Account] = {
     val needs = GroupMembers
       .filter(_.groupName === groupName.bind)
@@ -38,8 +49,13 @@ trait AnnounceService {
       }
     }
     .flatten
-    .filter(mail => EmailAddress.isValid(mail))
     .distinct
-    .toList
+    .partition(EmailAddress.isValid) match {
+      case (validMails, invalidMails) =>
+        if (invalidMails.nonEmpty) {
+          logger.warn("skipping malformed email address(es): {}", invalidMails.mkString(", "))
+        }
+        validMails.toList
+    }
   }
 }

src/test/scala/fr/brouillard/gitbucket/announce/service/AnnounceServiceSpec.scala

@@ -0,0 +1,57 @@
+package fr.brouillard.gitbucket.announce.service
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class AnnounceServiceSpec extends AnyFunSuite {
+
+  test("accepts plain addresses") {
+    assert(EmailAddress.isValid("user@example.com"))
+    assert(EmailAddress.isValid("USER@EXAMPLE.COM"))
+    assert(EmailAddress.isValid("user.name@sub.example.com"))
+  }
+
+  test("accepts addresses with an apostrophe in the local part") {
+    assert(EmailAddress.isValid("o'brien@example.com"))
+  }
+
+  test("accepts other RFC 5322 atext characters in the local part") {
+    assert(EmailAddress.isValid("user+tag@example.com"))
+    assert(EmailAddress.isValid("user_name@example.com"))
+    assert(EmailAddress.isValid("user-name@example.com"))
+  }
+
+  test("rejects addresses without an @") {
+    assert(!EmailAddress.isValid("not-an-email"))
+  }
+
+  test("rejects addresses with spaces") {
+    assert(!EmailAddress.isValid("user name@example.com"))
+  }
+
+  test("rejects empty string") {
+    assert(!EmailAddress.isValid(""))
+  }
+
+  // RFC 5322 dot-atom-text = 1*atext *("." 1*atext) -- "." is not itself an
+  // atext character, so it can only appear strictly between two non-empty
+  // atext runs: never leading, trailing, or doubled.
+  test("rejects a local part starting with a dot") {
+    assert(!EmailAddress.isValid(".user@example.com"))
+  }
+
+  test("rejects a local part ending with a dot") {
+    assert(!EmailAddress.isValid("user.@example.com"))
+  }
+
+  test("rejects a local part with consecutive dots") {
+    assert(!EmailAddress.isValid("user..name@example.com"))
+  }
+
+  // RFC 5322's domain grammar (dot-atom) doesn't forbid a label ending in a
+  // hyphen, even though such a label could never be a real DNS host name
+  // (RFC 1034/1123). isValid only checks RFC 5322 syntax, so this is accepted
+  // by design -- not something to "fix" by tightening the domain pattern.
+  test("accepts a domain label ending in a hyphen (RFC 5322 permits it, even though DNS wouldn't)") {
+    assert(EmailAddress.isValid("user@example-"))
+  }
+}