You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by bt...@apache.org on 2022/02/09 01:26:18 UTC
[james-project] 02/06: JAMES-3708 Stricter domain and address parsing
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 <bt...@linagora.com>
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,
"server-dev@james-apache.org",
- "a..b@domain.com",
"server-dev@[127.0.0.1]",
- "server-dev@#123",
- "server-dev@#123.apache.org",
"server.dev@james.apache.org",
- "\\.server-dev@james.apache.org",
- "server-dev\\.@james.apache.org")
+ "\\.server-dev@james.apache.org")
.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\\.@james.apache.org", // Javax.mail is unable to handle this so we better reject it
+ "a..b@domain.com")
.map(Arguments::of);
}
@@ -97,15 +97,6 @@ class MailAddressTest {
.doesNotThrowAnyException();
}
- @Disabled("JAMES-3708 Fails on the following values:" +
- "" +
- "a..b@domain.com -> javax.mail.internet.AddressException: Local address contains dot-dot in string ``a..b@domain.com''" +
- "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\\.@james.apache.org' -> javax.mail.internet.AddressException: Local address ends with dot in string ``server-dev\\.@james.apache.org''" +
- "" +
- "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",
+ "domain@bad.com",
+ "domain@bad.com",
+ "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 a..b@domain.com 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 {
+ /*
+ a..b@domain.com 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: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org