Email addresses validated despite cleared validation criteria

[ghost]

https://github.com/bbottema/simple-java-mail/blob/master/src/main/java/org/simplejavamail/mailer/Mailer.java#L275

The validate method tightly couples validation for injection attacks alongside email addresses validation. These are different types of validation. Consider:

public boolean validate(final Email email) {
  validateSenders(email);
  validateRecipients(email);
  validateAttacks(email);
}

This would allow for global settings to control validation. For example:

public boolean validate(final Email email) {
  if( validate() ) {
    validateSenders(email);
    validateRecipients(email);
    validateAttacks(email);
  }
}

public boolean validateSenders(final Email email) {
  if( validateSenders() ) {
    final InternetAddress[] addresses = email.getAddresses(FROM);
    if (!EmailAddressValidator.isValid(addresses, getEmailAddressCriteria())) {
      // ...
  }
}

[bbottema]

What's the use case for global control settings? Why not always check everything?

[ghost]

What's the use case for global control settings? Why not always check everything?

To override email validation of addresses (for I18N purposes as per #166) yet keep attack validations.

If there's a way to support I18N, as per the RFCs, then separating them is not a huge issue.

[bbottema]

This is already possible:

currentMailerBuilder
   .clearEmailAddressCriteria()
   (..)

This tells Simple Java Mail that there is no email address criteria to check and so will omit email address validation completely, yet keep the injection tests.

Does this solve your problem?

[bbottema]

@DaveJarvis, had any luck with this yet?

[ghost]

Didn't seem to help with the email address validation; if anything, clearing out the criteria made matters worse. Here's the code:

return MailerBuilder.withSMTPServer(host, port, username, password)
                    .withSessionTimeout(timeout)
                    .clearEmailAddressCriteria()
                    .buildMailer();

Here's the exception from the unit test:

testEmailSend_OnRecipientBodySubjectAttachment_ShouldDeliverMessageToRecipient(EmailNotificationServiceTest)  Time elapsed: 0.014 sec  <<< ERROR!
NotificationServiceException: org.simplejavamail.mailer.MailerException: Invalid TO address: Email{
	id=null
	fromRecipient=Recipient{name='Jane Doe', address='dev@cgi.com', type=null},
	replyToRecipient=null,
	bounceToRecipient=null,
	text='Hello, text! Voilà ! Le vieux château. À bientôt ! 一蓮托生',
	textHTML='null',
	subject='Text subject',
	recipients=[Recipient{name='null', address='dev@localhost', type=To}],
	attachments=[AttachmentResource{
		name='filename.txt',
		dataSource.name=,
		dataSource.getContentType=text/plain; charset=utf-8
	}]
}
	at org.simplejavamail.mailer.Mailer.validate(Mailer.java:280)
	at org.simplejavamail.mailer.Mailer.sendMail(Mailer.java:238)
	at org.simplejavamail.mailer.Mailer.sendMail(Mailer.java:230)

From Mailer.java, lines 278 to 280:

for (final Recipient recipient : email.getRecipients()) {
  if (!EmailAddressValidator.isValid(recipient.getAddress(), emailAddressCriteria)) {
    throw new MailerException(format(MailerException.INVALID_RECIPIENT, email));

So even with .clearEmailAddressCriteria() being called, the exception is now thrown for a perfectly valid email address. No email is sent.

When I remove the .clearEmailAddressCriteria() line, the unit test passes and the email is sent as expected.

In both cases (with or without the clear method call), I18N email addresses trigger an exception.

[ghost]

The reason is because of the following line:

https://github.com/bbottema/simple-java-mail/blob/master/src/main/java/org/simplejavamail/mailer/Mailer.java#L274

		} else if (emailAddressCriteria != null) {

There's no way to set the emailAddressCriteria to null.

Calling clearEmailAddressCriteria from https://github.com/bbottema/simple-java-mail/blob/master/src/main/java/org/simplejavamail/mailer/MailerGenericBuilder.java#L438 reveals:

	public T clearEmailAddressCriteria() {
		return withEmailAddressCriteria(EnumSet.noneOf(EmailAddressCriteria.class));
	}

This creates a non-null set. I'd recommend changing line Mailer.java:274 to check for null or an empty set, such as:

		} else if (emailAddressCriteria != null && !emailAddressCriteria.isEmpty()) {

Or, with a slight maintainability improvement:

		} else if (isEmpty(emailAddressCriteria)) {

With the addition of a boolean isEmpty(EnumSet ...) method that will determine whether or not the list is null/empty.

[ghost]

This is related to issue #121.

[bbottema]

Your analyses is spot on. Moreover emailAddressCriteria is garuanteed to be non-null, so the null-check is not only wrong, it is completely superfluous.

Released fix in 5.0.6, please confirm.