This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 5e1008d19b8453093d07cfde739b08d42b64d662 Author: Benoit Tellier <[email protected]> AuthorDate: Sun Jan 30 12:08:15 2022 +0700 JAMES-3708 Stricter domain and address parsing This avoids having down the line domains and email addresses we do not know handling. Note that this work is inspired from InternetDomainName with the modification that we allow domains being IP addresses. --- .../main/java/org/apache/james/core/Domain.java | 46 ++++++++- .../java/org/apache/james/core/MailAddress.java | 13 +++ .../org/apache/james/core/MailAddressTest.java | 23 ++--- .../apache/james/domainlist/api/DomainTest.java | 112 ++++++++++----------- ...ectResolutionRemoteDeliveryIntegrationTest.java | 15 +-- 5 files changed, 125 insertions(+), 84 deletions(-) diff --git a/core/src/main/java/org/apache/james/core/Domain.java b/core/src/main/java/org/apache/james/core/Domain.java index f4f1e37..7fb06d2 100644 --- a/core/src/main/java/org/apache/james/core/Domain.java +++ b/core/src/main/java/org/apache/james/core/Domain.java @@ -20,15 +20,24 @@ package org.apache.james.core; import java.io.Serializable; +import java.util.List; import java.util.Locale; import java.util.Objects; +import com.google.common.base.CharMatcher; import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; +import com.google.common.collect.Iterables; +import com.google.common.net.InetAddresses; public class Domain implements Serializable { + private static final CharMatcher DASH_MATCHER = CharMatcher.anyOf("-_"); + private static final CharMatcher DIGIT_MATCHER = CharMatcher.inRange('0', '9'); + private static final CharMatcher LETTER_MATCHER = CharMatcher.inRange('a', 'z').or(CharMatcher.inRange('A', 'Z')); + private static final CharMatcher PART_CHAR_MATCHER = DIGIT_MATCHER.or(LETTER_MATCHER).or(DASH_MATCHER); public static final Domain LOCALHOST = Domain.of("localhost"); - public static final int MAXIMUM_DOMAIN_LENGTH = 255; + public static final int MAXIMUM_DOMAIN_LENGTH = 253; private static String removeBrackets(String domainName) { if (!(domainName.startsWith("[") && domainName.endsWith("]"))) { @@ -39,11 +48,40 @@ public class Domain implements Serializable { public static Domain of(String domain) { Preconditions.checkNotNull(domain, "Domain can not be null"); - Preconditions.checkArgument(!domain.isEmpty() && !domain.contains("@") && !domain.contains("/"), - "Domain can not be empty nor contain `@` nor `/`"); Preconditions.checkArgument(domain.length() <= MAXIMUM_DOMAIN_LENGTH, "Domain name length should not exceed %s characters", MAXIMUM_DOMAIN_LENGTH); - return new Domain(domain); + + String domainWithoutBrackets = removeBrackets(domain); + List<String> parts = Splitter.on('.').splitToList(domainWithoutBrackets); + parts.forEach(Domain::assertValidPart); + assertValidLastPart(Iterables.getLast(parts), domainWithoutBrackets); + + return new Domain(domainWithoutBrackets); + } + + private static void assertValidPart(String domainPart) { + Preconditions.checkArgument(!domainPart.isEmpty(), "Domain part should not be empty"); + Preconditions.checkArgument(!DASH_MATCHER.matches(domainPart.charAt(0)), "Domain part should not start with '-' or '_'"); + Preconditions.checkArgument(!DASH_MATCHER.matches(domainPart.charAt(domainPart.length() - 1)), "Domain part should not end with '-' or '_'"); + Preconditions.checkArgument(domainPart.length() <= 63, "Domain part should not not exceed 63 characters"); + + String asciiChars = CharMatcher.ascii().retainFrom(domainPart); + Preconditions.checkArgument(PART_CHAR_MATCHER.matchesAllOf(asciiChars), "Domain parts ASCII chars must be a-z A-Z 0-9 - or _"); + } + + private static void assertValidLastPart(String domainPart, String domain) { + boolean onlyDigits = DIGIT_MATCHER.matches(domainPart.charAt(0)); + boolean invalid = onlyDigits && !validIPAddress(domain); + Preconditions.checkArgument(!invalid, "The k=last domain part must not start with 0-9"); + } + + private static boolean validIPAddress(String value) { + try { + InetAddresses.forString(value); + return true; + } catch (IllegalArgumentException e) { + return false; + } } private final String domainName; diff --git a/core/src/main/java/org/apache/james/core/MailAddress.java b/core/src/main/java/org/apache/james/core/MailAddress.java index beba6bf..2c67d79 100644 --- a/core/src/main/java/org/apache/james/core/MailAddress.java +++ b/core/src/main/java/org/apache/james/core/MailAddress.java @@ -29,6 +29,8 @@ import javax.mail.internet.InternetAddress; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.base.Splitter; + /** * A representation of an email address. * <p/> @@ -232,9 +234,20 @@ public class MailAddress implements java.io.Serializable { } localPart = localPartSB.toString(); + + if (localPart.startsWith(".") + || localPart.endsWith(".") + || haveDoubleDot(localPart)) { + throw new AddressException("Addresses cannot start end with '.' or contain two consecutive dots"); + } + domain = createDomain(domainSB.toString()); } + private boolean haveDoubleDot(String localPart) { + return Splitter.on('.').splitToList(localPart).stream().anyMatch(String::isEmpty); + } + private Domain createDomain(String domain) throws AddressException { try { return Domain.of(domain); diff --git a/core/src/test/java/org/apache/james/core/MailAddressTest.java b/core/src/test/java/org/apache/james/core/MailAddressTest.java index 91abe63..4c8624f 100644 --- a/core/src/test/java/org/apache/james/core/MailAddressTest.java +++ b/core/src/test/java/org/apache/james/core/MailAddressTest.java @@ -28,7 +28,6 @@ import javax.mail.internet.AddressException; import javax.mail.internet.InternetAddress; import org.assertj.core.api.Assertions; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -48,13 +47,9 @@ class MailAddressTest { GOOD_ADDRESS, GOOD_QUOTED_LOCAL_PART, "[email protected]", - "[email protected]", "server-dev@[127.0.0.1]", - "server-dev@#123", - "server-dev@#123.apache.org", "[email protected]", - "\\[email protected]", - "server-dev\\[email protected]") + "\\[email protected]") .map(Arguments::of); } @@ -85,8 +80,13 @@ class MailAddressTest { "server-dev@[0127.0.0.1]", "server-dev@[127.0.1.1a]", "server-dev@[127\\.0.1.1]", + "server-dev@#123", + "server-dev@#123.apache.org", "server-dev@[127.0.1.1.1]", - "server-dev@[127.0.1.-1]") + "server-dev@[127.0.1.-1]", + "\"a..b\"@domain.com", // Javax.mail is unable to handle this so we better reject it + "server-dev\\[email protected]", // Javax.mail is unable to handle this so we better reject it + "[email protected]") .map(Arguments::of); } @@ -97,15 +97,6 @@ class MailAddressTest { .doesNotThrowAnyException(); } - @Disabled("JAMES-3708 Fails on the following values:" + - "" + - "[email protected] -> javax.mail.internet.AddressException: Local address contains dot-dot in string ``[email protected]''" + - "server-dev@#123 -> javax.mail.internet.AddressException: Domain contains illegal character in string ``server-dev@#123''" + - "server-dev@#123.apache.org -> javax.mail.internet.AddressException: Domain contains illegal character in string ``server-dev@#123.apache.org'''" + - "server-dev\\[email protected]' -> javax.mail.internet.AddressException: Local address ends with dot in string ``server-dev\\[email protected]''" + - "" + - "Impact: potential NPEs (bouncing, remoteDelivery)" + - "Those values should likely be rejected") @ParameterizedTest @MethodSource("goodAddresses") void toInternetAddressShouldNoop(String mailAddress) throws Exception { diff --git a/core/src/test/java/org/apache/james/domainlist/api/DomainTest.java b/core/src/test/java/org/apache/james/domainlist/api/DomainTest.java index 3618943..c55393a 100644 --- a/core/src/test/java/org/apache/james/domainlist/api/DomainTest.java +++ b/core/src/test/java/org/apache/james/domainlist/api/DomainTest.java @@ -20,15 +20,19 @@ package org.apache.james.domainlist.api; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import org.apache.commons.lang3.StringUtils; import org.apache.james.core.Domain; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import nl.jqno.equalsverifier.EqualsVerifier; class DomainTest { + private static String DOMAIN_WITH_64_CHAR_PART = "abc." + "d".repeat(64) + ".com"; @Test void shouldRespectBeanContract() { @@ -42,64 +46,56 @@ class DomainTest { assertThat(Domain.of("Domain")).isEqualTo(Domain.of("domain")); } - @Test - void shouldRemoveBrackets() { - assertThat(Domain.of("[domain]")).isEqualTo(Domain.of("domain")); - } - - @Test - void openBracketWithTextShouldNotBeRemoved() { - assertThat(Domain.of("[domain")).isEqualTo(Domain.of("[Domain")); - } - - @Test - void singleOpenBracketShouldNotBeRemoved() { - assertThat(Domain.of("[")).isEqualTo(Domain.of("[")); - } - - @Test - void singleClosingBracketShouldNotBeRemoved() { - assertThat(Domain.of("]")).isEqualTo(Domain.of("]")); - } - - @Test - void closeBracketWithTextShouldNotBeRemoved() { - assertThat(Domain.of("aaa]")).isEqualTo(Domain.of("aaa]")); - } - - @Test - void bracketSurroundedWithTextShouldNotBeRemoved() { - assertThat(Domain.of("a[aaa]a")).isEqualTo(Domain.of("a[aaa]a")); - } - - @Test - void bracketWithTextSuffixShouldNotBeRemoved() { - assertThat(Domain.of("[aaa]a")).isEqualTo(Domain.of("[aaa]a")); - } - - @Test - void bracketWithTextPrefixShouldNotBeRemoved() { - assertThat(Domain.of("a[aaa]")).isEqualTo(Domain.of("a[aaa]")); - } - - @Test - void singleBracketShouldNotBeRemoved() { - assertThat(Domain.of("[]")).isEqualTo(Domain.of("[]")); - } - - @Test - void shouldThrowWhenDomainContainAtSymbol() { - assertThatThrownBy(() -> Domain.of("Dom@in")).isInstanceOf(IllegalArgumentException.class); + @ParameterizedTest + @ValueSource(strings = { + "", + "aab..ddd", + "aab.cc.1com", + "abc.abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcd.com", + "domain$bad.com", + "domain/bad.com", + "domain\\bad.com", + "[email protected]", + "[email protected]", + "domain%bad.com", + "#domain.com", + "bad-.com", + "bad_.com", + "-bad.com", + "bad_.com", + "[domain.tld", + "domain.tld]", + "a[aaa]a", + "[aaa]a", + "a[aaa]", + "[]" + }) + void invalidDomains(String arg) { + assertThatThrownBy(() -> Domain.of(arg)) + .isInstanceOf(IllegalArgumentException.class); } - - @Test - void shouldThrowWhenDomainContainUrlOperatorSymbol() { - assertThatThrownBy(() -> Domain.of("Dom/in")).isInstanceOf(IllegalArgumentException.class); + @ParameterizedTest + @ValueSource(strings = { + "domain.tld", + "do-main.tld", + "do_main.tld", + "ab.dc.de.fr", + "123.456.789.a23", + "acv.abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabc.fr", + "ab--cv.fr", + "ab__cd.fr", + "domain", + "[domain]", + "127.0.0.1" + }) + void validDomains(String arg) { + assertThatCode(() -> Domain.of(arg)) + .doesNotThrowAnyException(); } @Test - void shouldThrowWhenDomainIsEmpty() { - assertThatThrownBy(() -> Domain.of("")).isInstanceOf(IllegalArgumentException.class); + void shouldRemoveBrackets() { + assertThat(Domain.of("[domain]")).isEqualTo(Domain.of("domain")); } @Test @@ -108,14 +104,14 @@ class DomainTest { } @Test - void shouldAllow255LongDomain() { - assertThat(Domain.of(StringUtils.repeat('a', 255)).asString()) - .hasSize(255); + void shouldAllow253LongDomain() { + assertThat(Domain.of(StringUtils.repeat("aaaaaaaaa.", 25) + "aaa").asString()) + .hasSize(253); } @Test void shouldThrowWhenTooLong() { - assertThatThrownBy(() -> Domain.of(StringUtils.repeat('a', 256))) + assertThatThrownBy(() -> Domain.of(StringUtils.repeat('a', 254))) .isInstanceOf(IllegalArgumentException.class); } } \ No newline at end of file diff --git a/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/DirectResolutionRemoteDeliveryIntegrationTest.java b/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/DirectResolutionRemoteDeliveryIntegrationTest.java index 7e03f9c..9d1c7d7 100644 --- a/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/DirectResolutionRemoteDeliveryIntegrationTest.java +++ b/server/mailet/integration-testing/src/test/java/org/apache/james/mailets/DirectResolutionRemoteDeliveryIntegrationTest.java @@ -31,7 +31,6 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import java.io.File; -import java.io.IOException; import java.net.InetAddress; import java.util.List; @@ -49,6 +48,7 @@ import org.apache.james.transport.matchers.All; import org.apache.james.utils.DataProbeImpl; import org.apache.james.utils.FakeSmtp; import org.apache.james.utils.SMTPMessageSender; +import org.apache.james.utils.SMTPSendingException; import org.apache.james.utils.TestIMAPClient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Disabled; @@ -117,11 +117,13 @@ public class DirectResolutionRemoteDeliveryIntegrationTest { .untilAsserted(this::assertMessageReceivedByTheSmtpServer); } - @Disabled("JAMES-3708 [email protected] triggers a parsing error within javax.mail, the exception is ingnored and substituted with null," + - "resulting in a NPE in RemoteDelivery. Instead we should reject as part of SMTP reception emails we cannot handle." + - "This can be used to make the delivery of all remote recipients fail while local recipient succeeds.") @Test - void test(@TempDir File temporaryFolder) throws Exception { + void shouldRejectInvalidAddressesUponSubmission(@TempDir File temporaryFolder) throws Exception { + /* + [email protected] triggered a parsing error within javax.mail, the exception was ignored and substituted with null, + resulting in a NPE in RemoteDelivery. Instead we now reject it as part of SMTP reception emails we cannot handle. + This could be used to make the delivery of all remote recipients fail while local recipient succeeds. + */ InMemoryDNSService inMemoryDNSService = new InMemoryDNSService() .registerMxRecord(JAMES_ANOTHER_DOMAIN, fakeSmtp.getContainer().getContainerIp()); @@ -143,7 +145,8 @@ public class DirectResolutionRemoteDeliveryIntegrationTest { assertThatThrownBy(() -> messageSender.connect(LOCALHOST_IP, jamesServer.getProbe(SmtpGuiceProbe.class).getSmtpPort()) .sendMessage(FROM, "to..user@" + JAMES_ANOTHER_DOMAIN)) - .isInstanceOf(IOException.class); + .isInstanceOf(SMTPSendingException.class) + .hasMessageContaining("Error upon step RCPT: 553 5.1.3 Syntax error in recipient address"); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
