From f3abd96f4c923540dd1366418ca24b4b370d9d0e Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Fri, 30 Oct 2026 10:27:46 +0100 Subject: [PATCH 1/5] fix(settings): don't fail when a stored domain is added again Adding a trusted sender or an internal address only skipped the insert when the value was already covered by a stored entry. For domain entries that check compares the host of the given address, but a domain itself has no "@", so Horde finds no host and a stored domain was never found. The insert then failed on the unique index: the API answered with HTTP 500 and the user migration import aborted. Two parallel requests for the same new value failed the same way. Both services now treat a unique constraint violation on insert as "already stored", as the user migration already does for tags. exists() stays unchanged, because it also decides whether a sender is trusted when a message is shown. insertIgnoreConflict() isn't used, because on MySQL it runs INSERT IGNORE, which also hides truncated values. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner --- lib/Service/InternalAddressService.php | 20 ++- lib/Service/TrustedSenderService.php | 19 ++- .../Db/InternalAddressMapperTest.php | 13 ++ .../Db/TrustedSenderMapperTest.php | 13 ++ .../Service/InternalAddressServiceTest.php | 116 ++++++++++++++++++ .../Unit/Service/TrustedSenderServiceTest.php | 41 +++++++ 6 files changed, 212 insertions(+), 10 deletions(-) create mode 100644 tests/Unit/Service/InternalAddressServiceTest.php diff --git a/lib/Service/InternalAddressService.php b/lib/Service/InternalAddressService.php index bb06b27526..a367d00f7d 100644 --- a/lib/Service/InternalAddressService.php +++ b/lib/Service/InternalAddressService.php @@ -12,6 +12,7 @@ use OCA\Mail\Contracts\IInternalAddressService; use OCA\Mail\Db\InternalAddress; use OCA\Mail\Db\InternalAddressMapper; +use OCP\DB\Exception; class InternalAddressService implements IInternalAddressService { public function __construct( @@ -35,11 +36,20 @@ public function add(string $uid, string $address, string $type, ?bool $trust = t } if ($trust) { - $this->mapper->create( - $uid, - $address, - $type - ); + try { + $this->mapper->create( + $uid, + $address, + $type + ); + } catch (Exception $exception) { + if ($exception->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $exception; + } + + // isInternal() doesn't match a domain itself, and requests can race + return null; + } return $this->getInternalAddress($uid, $address); } else { $this->mapper->remove( diff --git a/lib/Service/TrustedSenderService.php b/lib/Service/TrustedSenderService.php index cc14101f0c..05afec06da 100644 --- a/lib/Service/TrustedSenderService.php +++ b/lib/Service/TrustedSenderService.php @@ -12,6 +12,7 @@ use OCA\Mail\Contracts\ITrustedSenderService; use OCA\Mail\Db\Message; use OCA\Mail\Db\TrustedSenderMapper; +use OCP\DB\Exception; class TrustedSenderService implements ITrustedSenderService { public function __construct( @@ -52,11 +53,19 @@ public function trust(string $uid, string $email, string $type, ?bool $trust = t } if ($trust) { - $this->mapper->create( - $uid, - $email, - $type - ); + try { + $this->mapper->create( + $uid, + $email, + $type + ); + } catch (Exception $exception) { + if ($exception->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $exception; + } + + // isTrusted() doesn't match a domain itself, and requests can race + } } else { $this->mapper->remove( $uid, diff --git a/tests/Integration/Db/InternalAddressMapperTest.php b/tests/Integration/Db/InternalAddressMapperTest.php index 469167eac6..b1488e13d2 100644 --- a/tests/Integration/Db/InternalAddressMapperTest.php +++ b/tests/Integration/Db/InternalAddressMapperTest.php @@ -14,6 +14,7 @@ use ChristophWurst\Nextcloud\Testing\TestUser; use OCA\Mail\Db\InternalAddress; use OCA\Mail\Db\InternalAddressMapper; +use OCP\DB\Exception; use OCP\IDBConnection; use OCP\IUser; use OCP\Server; @@ -153,6 +154,18 @@ public function testCreateDomain(): void { $this->assertCount(1, $rows); } + public function testCreateStoredDomainViolatesUniqueIndex(): void { + $uid = $this->user->getUID(); + $this->mapper->create($uid, 'example.test', 'domain'); + + try { + $this->mapper->create($uid, 'example.test', 'domain'); + $this->fail('Storing the same domain twice must fail'); + } catch (Exception $exception) { + $this->assertSame(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION, $exception->getReason()); + } + } + public function testRemoveDomain(): void { $uid = $this->user->getUID(); $qb = $this->db->getQueryBuilder(); diff --git a/tests/Integration/Db/TrustedSenderMapperTest.php b/tests/Integration/Db/TrustedSenderMapperTest.php index f5cc85f5a1..fa84ab7cf7 100644 --- a/tests/Integration/Db/TrustedSenderMapperTest.php +++ b/tests/Integration/Db/TrustedSenderMapperTest.php @@ -13,6 +13,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use ChristophWurst\Nextcloud\Testing\TestUser; use OCA\Mail\Db\TrustedSenderMapper; +use OCP\DB\Exception; use OCP\IDBConnection; use OCP\IUser; use OCP\Server; @@ -99,6 +100,18 @@ public function testCreateIndividual(): void { $this->assertCount(1, $rows); } + public function testCreateStoredDomainViolatesUniqueIndex(): void { + $uid = $this->user->getUID(); + $this->mapper->create($uid, 'example.test', 'domain'); + + try { + $this->mapper->create($uid, 'example.test', 'domain'); + $this->fail('Storing the same domain twice must fail'); + } catch (Exception $exception) { + $this->assertSame(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION, $exception->getReason()); + } + } + public function testRemoveIndividual(): void { $uid = $this->user->getUID(); $qb = $this->db->getQueryBuilder(); diff --git a/tests/Unit/Service/InternalAddressServiceTest.php b/tests/Unit/Service/InternalAddressServiceTest.php new file mode 100644 index 0000000000..9aba1519df --- /dev/null +++ b/tests/Unit/Service/InternalAddressServiceTest.php @@ -0,0 +1,116 @@ +mapper = $this->createMock(InternalAddressMapper::class); + + $this->service = new InternalAddressService( + $this->mapper + ); + } + + public function testAddAlreadyInternal(): void { + $this->mapper->expects($this->once()) + ->method('exists') + ->with('alice', 'bob@example.test') + ->willReturn(true); + $this->mapper->expects($this->never()) + ->method('create'); + $this->mapper->expects($this->never()) + ->method('remove'); + + $internalAddress = $this->service->add('alice', 'bob@example.test', 'individual'); + + $this->assertNull($internalAddress); + } + + public function testAddNew(): void { + $created = new InternalAddress(); + $created->setUserId('alice'); + $created->setAddress('example.test'); + $created->setType('domain'); + $this->mapper->expects($this->once()) + ->method('exists') + ->with('alice', 'example.test') + ->willReturn(false); + $this->mapper->expects($this->once()) + ->method('create') + ->with('alice', 'example.test', 'domain') + ->willReturn(1); + $this->mapper->expects($this->once()) + ->method('find') + ->with('alice', 'example.test') + ->willReturn($created); + + $internalAddress = $this->service->add('alice', 'example.test', 'domain'); + + $this->assertSame($created, $internalAddress); + } + + public function testAddAlreadyStoredDomain(): void { + $uniqueViolation = $this->createMock(Exception::class); + $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); + $this->mapper->expects($this->once()) + ->method('exists') + ->with('alice', 'example.test') + ->willReturn(false); + $this->mapper->expects($this->once()) + ->method('create') + ->with('alice', 'example.test', 'domain') + ->willThrowException($uniqueViolation); + $this->mapper->expects($this->never()) + ->method('find'); + + $internalAddress = $this->service->add('alice', 'example.test', 'domain'); + + $this->assertNull($internalAddress); + } + + public function testAddRethrowsOtherDatabaseErrors(): void { + $databaseError = $this->createMock(Exception::class); + $databaseError->method('getReason')->willReturn(Exception::REASON_CONNECTION_LOST); + $this->mapper->method('exists') + ->willReturn(false); + $this->mapper->method('create') + ->willThrowException($databaseError); + + $this->expectExceptionObject($databaseError); + + $this->service->add('alice', 'example.test', 'domain'); + } + + public function testRemove(): void { + $this->mapper->expects($this->never()) + ->method('exists'); + $this->mapper->expects($this->never()) + ->method('create'); + $this->mapper->expects($this->once()) + ->method('remove') + ->with('alice', 'example.test', 'domain'); + + $internalAddress = $this->service->add('alice', 'example.test', 'domain', false); + + $this->assertNull($internalAddress); + } +} diff --git a/tests/Unit/Service/TrustedSenderServiceTest.php b/tests/Unit/Service/TrustedSenderServiceTest.php index 1cfc440179..044d535489 100644 --- a/tests/Unit/Service/TrustedSenderServiceTest.php +++ b/tests/Unit/Service/TrustedSenderServiceTest.php @@ -12,6 +12,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use OCA\Mail\Db\TrustedSenderMapper; use OCA\Mail\Service\TrustedSenderService; +use OCP\DB\Exception; use PHPUnit\Framework\MockObject\MockObject; class TrustedSenderServiceTest extends TestCase { @@ -98,6 +99,46 @@ public function testTrustNew(): void { ); } + public function testTrustAlreadyStoredDomain(): void { + $uid = 'alice'; + $domain = 'example.test'; + $uniqueViolation = $this->createMock(Exception::class); + $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); + $this->mapper->expects($this->once()) + ->method('exists') + ->with($uid, $domain) + ->willReturn(false); + $this->mapper->expects($this->once()) + ->method('create') + ->with($uid, $domain, 'domain') + ->willThrowException($uniqueViolation); + + $this->service->trust( + $uid, + $domain, + 'domain' + ); + } + + public function testTrustRethrowsOtherDatabaseErrors(): void { + $uid = 'alice'; + $domain = 'example.test'; + $databaseError = $this->createMock(Exception::class); + $databaseError->method('getReason')->willReturn(Exception::REASON_CONNECTION_LOST); + $this->mapper->method('exists') + ->willReturn(false); + $this->mapper->method('create') + ->willThrowException($databaseError); + + $this->expectExceptionObject($databaseError); + + $this->service->trust( + $uid, + $domain, + 'domain' + ); + } + public function testRemoveTrust(): void { $uid = 'greta'; $email = 'christoph@next.cloud'; From 623da79d1e99b158be6144c9c4bcc07c3124510f Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Fri, 30 Oct 2026 13:11:32 +0100 Subject: [PATCH 2/5] fix(settings): reject invalid addresses and domains Trusted senders and internal addresses accepted any text and any type, through the API as well as the user migration import, so entries that can never match a sender were stored. A new entry now has to be an address by RFC 5322 and RFC 6532 for the type "individual", parsed with Horde's strict mode, or a host name (RFC 1123, internationalized by IDNA2008 with the UTS #46 mapping) or an address literal (RFC 5321) for the type "domain". Horde's strict mode accepts a trailing dot, over-long labels, any address literal, invalid UTF-8 and control characters, so the encoding and the domain part are checked separately. Control characters, invisible format characters and line or paragraph separators are rejected, although RFC 5322 and RFC 6532 allow some of them, because they can break the UI or disguise an address. The zero-width joiner and non-joiner stay allowed, because names in Persian or Indian languages such as Hindi need them. Entries are stored in the form Horde produces for a sender's address (quotes and brackets removed, the domain lowercased and IDN-decoded), because that's the form senders are compared with. Domains are mapped by UTS #46 first, so other spellings of the same domain match as well. The stored form is limited to the 254 octets of an RFC 5321 path, since IDN decoding can lengthen it and Oracle's columns count bytes; very long internationalized domains are rejected because of that. Invalid input is answered with HTTP 422, and the import skips such entries with its existing notice. Senders whose local part needs its quotes, or with an IPv6 literal, can't be trusted individually anymore, because their compared form isn't a valid address. Trusting their domain still works. Removing an entry isn't validated, so existing invalid entries can still be deleted; they aren't cleaned up automatically. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner --- lib/Contracts/IInternalAddressService.php | 4 + lib/Contracts/ITrustedSenderService.php | 4 + lib/Db/AddressEntryType.php | 15 + lib/Db/InternalAddressMapper.php | 4 +- lib/Db/TrustedSenderMapper.php | 4 +- lib/Exception/InvalidAddressException.php | 19 ++ lib/Service/AddressEntryNormalizer.php | 148 ++++++++++ lib/Service/InternalAddressService.php | 27 +- lib/Service/TrustedSenderService.php | 27 +- .../InternalAddressesMigrationService.php | 23 +- .../TrustedSendersMigrationService.php | 23 +- .../Exception/InvalidAddressExceptionTest.php | 22 ++ .../Service/AddressEntryNormalizerTest.php | 265 ++++++++++++++++++ .../Service/InternalAddressServiceTest.php | 128 ++++++++- .../Unit/Service/TrustedSenderServiceTest.php | 116 +++++++- .../InternalAddressesMigrationServiceTest.php | 27 ++ .../TrustedSendersMigrationServiceTest.php | 26 ++ 17 files changed, 852 insertions(+), 30 deletions(-) create mode 100644 lib/Db/AddressEntryType.php create mode 100644 lib/Exception/InvalidAddressException.php create mode 100644 lib/Service/AddressEntryNormalizer.php create mode 100644 tests/Unit/Exception/InvalidAddressExceptionTest.php create mode 100644 tests/Unit/Service/AddressEntryNormalizerTest.php diff --git a/lib/Contracts/IInternalAddressService.php b/lib/Contracts/IInternalAddressService.php index 9dc69747cb..d6c1746963 100644 --- a/lib/Contracts/IInternalAddressService.php +++ b/lib/Contracts/IInternalAddressService.php @@ -10,10 +10,14 @@ namespace OCA\Mail\Contracts; use OCA\Mail\Db\InternalAddress; +use OCA\Mail\Exception\InvalidAddressException; interface IInternalAddressService { public function isInternal(string $uid, string $address): bool; + /** + * @throws InvalidAddressException if the $address to add or its $type is invalid + */ public function add(string $uid, string $address, string $type, ?bool $trust = true); /** diff --git a/lib/Contracts/ITrustedSenderService.php b/lib/Contracts/ITrustedSenderService.php index 06df620c4a..7dd0117153 100644 --- a/lib/Contracts/ITrustedSenderService.php +++ b/lib/Contracts/ITrustedSenderService.php @@ -10,10 +10,14 @@ namespace OCA\Mail\Contracts; use OCA\Mail\Db\TrustedSender; +use OCA\Mail\Exception\InvalidAddressException; interface ITrustedSenderService { public function isTrusted(string $uid, string $email): bool; + /** + * @throws InvalidAddressException if the $email to trust or its $type is invalid + */ public function trust(string $uid, string $email, string $type, ?bool $trust = true); /** diff --git a/lib/Db/AddressEntryType.php b/lib/Db/AddressEntryType.php new file mode 100644 index 0000000000..d025f588d5 --- /dev/null +++ b/lib/Db/AddressEntryType.php @@ -0,0 +1,15 @@ +expr()->orX( $qb->expr()->andX( $qb->expr()->eq('address', $qb->createNamedParameter($address)), - $qb->expr()->eq('type', $qb->createNamedParameter('individual')) + $qb->expr()->eq('type', $qb->createNamedParameter(AddressEntryType::Individual->value)) ), $qb->expr()->andX( $qb->expr()->eq('address', $qb->createNamedParameter($host)), - $qb->expr()->eq('type', $qb->createNamedParameter('domain')) + $qb->expr()->eq('type', $qb->createNamedParameter(AddressEntryType::Domain->value)) ) ), $qb->expr()->eq('user_id', $qb->createNamedParameter($uid)) diff --git a/lib/Db/TrustedSenderMapper.php b/lib/Db/TrustedSenderMapper.php index 791e81a3c9..9c38ea74b8 100644 --- a/lib/Db/TrustedSenderMapper.php +++ b/lib/Db/TrustedSenderMapper.php @@ -31,11 +31,11 @@ public function exists(string $uid, string $email): bool { $qb->expr()->orX( $qb->expr()->andX( $qb->expr()->eq('email', $qb->createNamedParameter($email)), - $qb->expr()->eq('type', $qb->createNamedParameter('individual')) + $qb->expr()->eq('type', $qb->createNamedParameter(AddressEntryType::Individual->value)) ), $qb->expr()->andX( $qb->expr()->eq('email', $qb->createNamedParameter($host)), - $qb->expr()->eq('type', $qb->createNamedParameter('domain')) + $qb->expr()->eq('type', $qb->createNamedParameter(AddressEntryType::Domain->value)) ) ), $qb->expr()->eq('user_id', $qb->createNamedParameter($uid)) diff --git a/lib/Exception/InvalidAddressException.php b/lib/Exception/InvalidAddressException.php new file mode 100644 index 0000000000..65fbc36f20 --- /dev/null +++ b/lib/Exception/InvalidAddressException.php @@ -0,0 +1,19 @@ + self::MAX_LENGTH) { + throw new InvalidAddressException('Email address or domain too long'); + } + + $normalized = match ($type) { + AddressEntryType::Individual => $this->normalizeAddress($value), + AddressEntryType::Domain => $this->normalizeDomain($value), + }; + + // IDN decoding can lengthen the value, and Oracle's columns count bytes + if (strlen($normalized) > self::MAX_LENGTH) { + throw new InvalidAddressException('Email address or domain too long'); + } + + return $normalized; + } + + /** + * @return string|null the stored form of $value, or null if $value or $type is invalid + */ + public function tryNormalize(string $value, string $type): ?string { + $entryType = AddressEntryType::tryFrom($type); + if ($entryType === null) { + return null; + } + + try { + return $this->normalize($value, $entryType); + } catch (InvalidAddressException) { + return null; + } + } + + /** + * @throws InvalidAddressException + */ + private function normalizeAddress(string $value): string { + if (!mb_check_encoding($value, 'UTF-8') + || preg_match(self::DISALLOWED_CHARACTERS, $value) === 1) { + throw new InvalidAddressException('Invalid email address'); + } + + $separator = strrpos($value, '@'); + if ($separator === false || $separator === 0) { + throw new InvalidAddressException('Invalid email address'); + } + + $localPart = substr($value, 0, $separator); + $domain = substr($value, $separator + 1); + // Horde accepts a trailing dot, over-long labels and any address literal + $host = $this->toAsciiHost($domain); + if ($host === null) { + throw new InvalidAddressException('Invalid email address'); + } + + $addresses = $this->parseAddressList($value) + // The stored form of a sender drops the quotes around its local part, e.g. of "alice smith"@example.test + ?? $this->parseAddressList('"' . addcslashes($localPart, '"\\') . '"@' . $domain); + $address = $addresses !== null && count($addresses) === 1 ? $addresses->first() : null; + if (!($address instanceof Horde_Mail_Rfc822_Address)) { + throw new InvalidAddressException('Invalid email address'); + } + + $address->host = $host; + return $address->bare_address; + } + + private function parseAddressList(string $value): ?Horde_Mail_Rfc822_List { + try { + // RFC 5322, with the UTF-8 addresses of RFC 6532 + return (new Horde_Mail_Rfc822())->parseAddressList($value, ['validate' => 'eai']); + } catch (Horde_Mail_Exception) { + return null; + } + } + + /** + * @throws InvalidAddressException + */ + private function normalizeDomain(string $domain): string { + $host = $this->toAsciiHost($domain); + if ($host === null) { + throw new InvalidAddressException('Invalid domain'); + } + + // Same form as the host of a parsed sender: IDN-decoded and lowercased by Horde + $address = new Horde_Mail_Rfc822_Address(); + $address->host = $host; + return $address->host; + } + + private function toAsciiHost(string $domain): ?string { + if (str_starts_with($domain, '[') && str_ends_with($domain, ']')) { + // RFC 5321, section 4.1.3, without IPv6 address literals: Horde can't parse a sender with one + $literal = substr($domain, 1, -1); + return filter_var($literal, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) !== false ? $literal : null; + } + + if ($domain === '') { + return null; + } + + $asciiDomain = idn_to_ascii($domain, self::IDNA_OPTIONS, INTL_IDNA_VARIANT_UTS46); + // UTS #46 accepts a trailing dot, RFC 5322 doesn't + if ($asciiDomain === false || str_ends_with($asciiDomain, '.')) { + return null; + } + + // RFC 3696, section 2: only an IPv4 address ends with an all-numeric label + if (preg_match('/(^|\.)[0-9]+$/', $asciiDomain) === 1 + && filter_var($asciiDomain, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) === false) { + return null; + } + + return $asciiDomain; + } +} diff --git a/lib/Service/InternalAddressService.php b/lib/Service/InternalAddressService.php index a367d00f7d..0566bc1093 100644 --- a/lib/Service/InternalAddressService.php +++ b/lib/Service/InternalAddressService.php @@ -10,13 +10,16 @@ namespace OCA\Mail\Service; use OCA\Mail\Contracts\IInternalAddressService; +use OCA\Mail\Db\AddressEntryType; use OCA\Mail\Db\InternalAddress; use OCA\Mail\Db\InternalAddressMapper; +use OCA\Mail\Exception\InvalidAddressException; use OCP\DB\Exception; class InternalAddressService implements IInternalAddressService { public function __construct( private InternalAddressMapper $mapper, + private AddressEntryNormalizer $normalizer, ) { } @@ -28,14 +31,19 @@ public function isInternal(string $uid, string $address): bool { ); } + /** + * @throws InvalidAddressException + */ #[\Override] public function add(string $uid, string $address, string $type, ?bool $trust = true): ?InternalAddress { - if ($trust && $this->isInternal($uid, $address)) { - // Nothing to do - return null; - } - if ($trust) { + $entryType = AddressEntryType::tryFrom($type) ?? throw new InvalidAddressException('Invalid type'); + $address = $this->normalizer->normalize($address, $entryType); + if ($this->isInternal($uid, $address)) { + // Nothing to do + return null; + } + try { $this->mapper->create( $uid, @@ -57,6 +65,15 @@ public function add(string $uid, string $address, string $type, ?bool $trust = t $address, $type ); + // Valid values are stored normalized, older entries as they were entered + $normalized = $this->normalizer->tryNormalize($address, $type); + if ($normalized !== null && $normalized !== $address) { + $this->mapper->remove( + $uid, + $normalized, + $type + ); + } } return null; } diff --git a/lib/Service/TrustedSenderService.php b/lib/Service/TrustedSenderService.php index 05afec06da..7f816b77d8 100644 --- a/lib/Service/TrustedSenderService.php +++ b/lib/Service/TrustedSenderService.php @@ -10,13 +10,16 @@ namespace OCA\Mail\Service; use OCA\Mail\Contracts\ITrustedSenderService; +use OCA\Mail\Db\AddressEntryType; use OCA\Mail\Db\Message; use OCA\Mail\Db\TrustedSenderMapper; +use OCA\Mail\Exception\InvalidAddressException; use OCP\DB\Exception; class TrustedSenderService implements ITrustedSenderService { public function __construct( private TrustedSenderMapper $mapper, + private AddressEntryNormalizer $normalizer, ) { } @@ -45,14 +48,19 @@ public function isSenderTrusted(string $uid, Message $message): bool { ); } + /** + * @throws InvalidAddressException + */ #[\Override] public function trust(string $uid, string $email, string $type, ?bool $trust = true): void { - if ($trust && $this->isTrusted($uid, $email)) { - // Nothing to do - return; - } - if ($trust) { + $entryType = AddressEntryType::tryFrom($type) ?? throw new InvalidAddressException('Invalid type'); + $email = $this->normalizer->normalize($email, $entryType); + if ($this->isTrusted($uid, $email)) { + // Nothing to do + return; + } + try { $this->mapper->create( $uid, @@ -72,6 +80,15 @@ public function trust(string $uid, string $email, string $type, ?bool $trust = t $email, $type ); + // Valid values are stored normalized, older entries as they were entered + $normalized = $this->normalizer->tryNormalize($email, $type); + if ($normalized !== null && $normalized !== $email) { + $this->mapper->remove( + $uid, + $normalized, + $type + ); + } } } diff --git a/lib/UserMigration/Service/InternalAddressesMigrationService.php b/lib/UserMigration/Service/InternalAddressesMigrationService.php index 47ecbe94c9..eb718faa32 100644 --- a/lib/UserMigration/Service/InternalAddressesMigrationService.php +++ b/lib/UserMigration/Service/InternalAddressesMigrationService.php @@ -11,6 +11,7 @@ use JsonException; use OCA\Mail\Contracts\IInternalAddressService; +use OCA\Mail\Exception\InvalidAddressException; use OCA\Mail\UserMigration\MailAccountMigrator; use OCP\IL10N; use OCP\IUser; @@ -106,20 +107,28 @@ public function importInternalAddresses(IUser $user, IImportSource $importSource foreach ($internalAddresses as $internalAddress) { if (!$this->isWellFormedInternalAddress($internalAddress)) { - $output->writeln( - $this->l10n->t( - 'An internal address of %s is invalid and will be skipped. Continue …', - [$user->getUID()] - ) - ); + $this->reportInvalidInternalAddress($user, $output); continue; } - $this->internalAddressService->add($user->getUID(), $internalAddress['address'], $internalAddress['type']); + try { + $this->internalAddressService->add($user->getUID(), $internalAddress['address'], $internalAddress['type']); + } catch (InvalidAddressException) { + $this->reportInvalidInternalAddress($user, $output); + } } } + private function reportInvalidInternalAddress(IUser $user, OutputInterface $output): void { + $output->writeln( + $this->l10n->t( + 'An internal address of %s is invalid and will be skipped. Continue …', + [$user->getUID()] + ) + ); + } + /** * Validate that the parsed internal addresses are a list. * diff --git a/lib/UserMigration/Service/TrustedSendersMigrationService.php b/lib/UserMigration/Service/TrustedSendersMigrationService.php index 3b50388ad6..044a9a8890 100644 --- a/lib/UserMigration/Service/TrustedSendersMigrationService.php +++ b/lib/UserMigration/Service/TrustedSendersMigrationService.php @@ -11,6 +11,7 @@ use JsonException; use OCA\Mail\Contracts\ITrustedSenderService; +use OCA\Mail\Exception\InvalidAddressException; use OCA\Mail\UserMigration\MailAccountMigrator; use OCP\IL10N; use OCP\IUser; @@ -100,12 +101,7 @@ public function importTrustedSenders(IUser $user, IImportSource $importSource, O foreach ($trustedSenders as $trustedSender) { if (!$this->isWellFormedTrustedSender($trustedSender)) { - $output->writeln( - $this->l10n->t( - 'A trusted sender of %s is invalid and will be skipped. Continue …', - [$user->getUID()] - ) - ); + $this->reportInvalidTrustedSender($user, $output); continue; } @@ -117,10 +113,23 @@ public function importTrustedSenders(IUser $user, IImportSource $importSource, O ), OutputInterface::VERBOSITY_VERBOSE ); - $this->trustedSenderService->trust($user->getUID(), $trustedSender['email'], $trustedSender['type']); + try { + $this->trustedSenderService->trust($user->getUID(), $trustedSender['email'], $trustedSender['type']); + } catch (InvalidAddressException) { + $this->reportInvalidTrustedSender($user, $output); + } } } + private function reportInvalidTrustedSender(IUser $user, OutputInterface $output): void { + $output->writeln( + $this->l10n->t( + 'A trusted sender of %s is invalid and will be skipped. Continue …', + [$user->getUID()] + ) + ); + } + /** * Validate that the parsed trusted senders are a list. * diff --git a/tests/Unit/Exception/InvalidAddressExceptionTest.php b/tests/Unit/Exception/InvalidAddressExceptionTest.php new file mode 100644 index 0000000000..4c7e2f0517 --- /dev/null +++ b/tests/Unit/Exception/InvalidAddressExceptionTest.php @@ -0,0 +1,22 @@ +assertSame(Http::STATUS_UNPROCESSABLE_ENTITY, $exception->getHttpCode()); + } +} diff --git a/tests/Unit/Service/AddressEntryNormalizerTest.php b/tests/Unit/Service/AddressEntryNormalizerTest.php new file mode 100644 index 0000000000..93a5e88397 --- /dev/null +++ b/tests/Unit/Service/AddressEntryNormalizerTest.php @@ -0,0 +1,265 @@ +normalizer = new AddressEntryNormalizer(); + } + + public static function provideValidAddresses(): array { + return [ + 'plain address' => ['alice@example.test', 'alice@example.test'], + 'host without a dot' => ['root@localhost', 'root@localhost'], + 'IPv4 address' => ['bob@192.168.0.1', 'bob@192.168.0.1'], + 'IPv4 address literal' => ['bob@[192.168.0.1]', 'bob@192.168.0.1'], + 'quoted local part' => ['"alice smith"@example.test', 'alice smith@example.test'], + 'internationalized address' => ['jöran@bücher.example', 'jöran@bücher.example'], + 'punycode domain' => ['alice@xn--bcher-kva.example', 'alice@bücher.example'], + 'decomposed domain' => ["alice@bu\u{0308}cher.example", 'alice@bücher.example'], + 'fullwidth full stop' => ["alice@example\u{FF0E}test", 'alice@example.test'], + 'upper case' => ['ALICE@Example.TEST', 'ALICE@example.test'], + 'digit zero' => ['alice0@example0.test', 'alice0@example0.test'], + 'zero width non-joiner' => ["\u{0639}\u{0644}\u{06CC}\u{200C}\u{0631}\u{0636}\u{0627}@example.test", "\u{0639}\u{0644}\u{06CC}\u{200C}\u{0631}\u{0636}\u{0627}@example.test"], + 'zero width joiner' => ["\u{0915}\u{094D}\u{200D}\u{0937}@example.test", "\u{0915}\u{094D}\u{200D}\u{0937}@example.test"], + 'local part over 64 characters' => [str_repeat('a', 65) . '@example.test', str_repeat('a', 65) . '@example.test'], + // The stored form of a sender keeps a local part that needs quotes without them + 'unquoted space' => ['alice smith@example.test', 'alice smith@example.test'], + 'unquoted spaces' => ['apple.com eqe@example.test', 'apple.com eqe@example.test'], + 'unquoted parentheses' => ['(work)alice@example.test', '(work)alice@example.test'], + 'unquoted at sign' => ['alice@bob@example.test', 'alice@bob@example.test'], + 'unquoted quotation mark' => ['a"b@example.test', 'a"b@example.test'], + ]; + } + + /** + * @dataProvider provideValidAddresses + */ + public function testNormalizesValidAddress(string $address, string $expected): void { + $normalized = $this->normalizer->normalize($address, AddressEntryType::Individual); + + $this->assertSame($expected, $normalized); + } + + public static function provideInvalidAddresses(): array { + return [ + 'empty' => [''], + 'missing local part' => ['@example.test'], + 'missing at sign' => ['alice'], + 'missing domain' => ['alice@'], + 'trailing dot' => ['alice@example.test.'], + 'label over 63 characters' => ['alice@' . str_repeat('b', 64) . '.test'], + 'display name' => ['Alice '], + 'two addresses' => ['alice@example.test, bob@example.test'], + 'group' => ['friends: alice@example.test;'], + 'trailing comment' => ['alice@example.test (work)'], + 'space after the at sign' => ['alice@ example.test'], + 'invalid UTF-8' => ["al\xFFice@example.test"], + 'null character' => ["\"alice\x00smith\"@example.test"], + 'line break' => ["\"alice\r\nsmith\"@example.test"], + 'delete character' => ["\"alice\x7Fsmith\"@example.test"], + 'tab' => ["\"alice\tsmith\"@example.test"], + 'C1 control character' => ["\"alice\u{0085}smith\"@example.test"], + 'line separator' => ["alice\u{2028}smith@example.test"], + 'paragraph separator' => ["alice\u{2029}smith@example.test"], + 'right-to-left override' => ["alice\u{202E}smith@example.test"], + 'zero width space' => ["alice\u{200B}smith@example.test"], + 'invalid IPv4 literal' => ['bob@[300.1.1.1]'], + 'IPv6 address literal' => ['bob@[IPv6:2001:db8::1]'], + 'all-numeric top-level domain' => ['alice@example.123'], + ]; + } + + /** + * @dataProvider provideInvalidAddresses + */ + public function testRejectsInvalidAddress(string $address): void { + $this->expectException(InvalidAddressException::class); + $this->expectExceptionMessage('Invalid email address'); + + $this->normalizer->normalize($address, AddressEntryType::Individual); + } + + public static function provideValidDomains(): array { + return [ + 'domain' => ['example.test', 'example.test'], + 'subdomain' => ['mail.example.test', 'mail.example.test'], + 'host without a dot' => ['localhost', 'localhost'], + 'upper case' => ['Example.TEST', 'example.test'], + 'internationalized domain' => ['bücher.example', 'bücher.example'], + 'punycode domain' => ['xn--bcher-kva.example', 'bücher.example'], + 'upper-case punycode domain' => ['XN--BCHER-KVA.example', 'bücher.example'], + 'decomposed domain' => ["bu\u{0308}cher.example", 'bücher.example'], + 'fullwidth full stop' => ["example\u{FF0E}test", 'example.test'], + 'right-to-left domain' => ["\u{05D0}\u{05D1}.example", "\u{05D0}\u{05D1}.example"], + 'IPv4 address' => ['192.168.0.1', '192.168.0.1'], + 'IPv4 address literal' => ['[192.168.0.1]', '192.168.0.1'], + ]; + } + + /** + * @dataProvider provideValidDomains + */ + public function testNormalizesValidDomain(string $domain, string $expected): void { + $normalized = $this->normalizer->normalize($domain, AddressEntryType::Domain); + + $this->assertSame($expected, $normalized); + } + + public static function provideInvalidDomains(): array { + return [ + 'spaces' => ['not valid'], + 'empty' => [''], + 'trailing dot' => ['example.test.'], + 'trailing ideographic full stop' => ["example.test\u{3002}"], + 'leading dot' => ['.example.test'], + 'leading at sign' => ['@example.test'], + 'email address' => ['alice@example.test'], + 'underscore' => ['exa_mple.test'], + 'leading hyphen' => ['-example.test'], + 'label over 63 characters' => [str_repeat('b', 64) . '.test'], + 'mixed text directions' => ["a\u{05D0}b.example"], + 'zero width joiner' => ["a\u{200D}b.example"], + 'invalid IPv4 literal' => ['[300.1.1.1]'], + 'IPv6 address literal' => ['[IPv6:2001:db8::1]'], + 'IPv6 address literal as stored before' => ['ipv6:2001:db8::1'], + 'IPv6 literal without the tag' => ['[2001:db8::1]'], + 'domain in brackets' => ['[example.test]'], + 'all-numeric top-level domain' => ['example.123'], + 'numeric labels that are no IPv4 address' => ['999.1.1.1'], + 'single numeric label' => ['123'], + ]; + } + + /** + * @dataProvider provideInvalidDomains + */ + public function testRejectsInvalidDomain(string $domain): void { + $this->expectException(InvalidAddressException::class); + $this->expectExceptionMessage('Invalid domain'); + + $this->normalizer->normalize($domain, AddressEntryType::Domain); + } + + public function testTryNormalizeReturnsTheStoredForm(): void { + $this->assertSame('bücher.example', $this->normalizer->tryNormalize('xn--bcher-kva.example', 'domain')); + } + + public static function provideInvalidEntries(): array { + return [ + 'invalid domain' => ['not valid', 'domain'], + 'invalid address' => ['alice', 'individual'], + 'unknown type' => ['example.test', 'group'], + ]; + } + + /** + * @dataProvider provideInvalidEntries + */ + public function testTryNormalizeReturnsNullForAnInvalidEntry(string $value, string $type): void { + $this->assertNull($this->normalizer->tryNormalize($value, $type)); + } + + public function testAcceptsAStoredFormOf254Octets(): void { + $domain = self::internationalizedDomain(254); + + $normalized = $this->normalizer->normalize($domain, AddressEntryType::Domain); + + $this->assertSame($domain, $normalized); + } + + public static function provideValuesOver254Octets(): array { + return [ + // RFC 5321 limits a path to 256 octets including the angle brackets, which leaves 254 + 'address of 255 octets' => [str_repeat('a', 242) . '@example.test', AddressEntryType::Individual], + 'domain of 255 octets' => [self::internationalizedDomain(255), AddressEntryType::Domain], + // Each "．" (U+FF0E, 3 octets) becomes ".", so only the value as given is too long + 'domain that normalizing shortens' => [str_repeat("a\u{FF0E}", 85) . 'test', AddressEntryType::Domain], + ]; + } + + /** + * @dataProvider provideValuesOver254Octets + */ + public function testRejectsAValueOver254Octets(string $value, AddressEntryType $type): void { + $this->expectException(InvalidAddressException::class); + $this->expectExceptionMessage('Email address or domain too long'); + + $this->normalizer->normalize($value, $type); + } + + public static function provideValuesStoredWithOver254Octets(): array { + // UTS #46 maps "㌀" (U+3300, 3 octets) to "アパート" (12 octets) + $label = str_repeat("\u{3300}", 12); + return [ + 'domain of 118 octets that UTS #46 lengthens' => ["$label.$label.$label.example", AddressEntryType::Domain], + 'address of 124 octets that UTS #46 lengthens' => ["alice@$label.$label.$label.example", AddressEntryType::Individual], + ]; + } + + /** + * @dataProvider provideValuesStoredWithOver254Octets + */ + public function testRejectsAStoredFormOver254Octets(string $value, AddressEntryType $type): void { + $this->expectException(InvalidAddressException::class); + $this->expectExceptionMessage('Email address or domain too long'); + + $this->normalizer->normalize($value, $type); + } + + public static function provideSenders(): array { + return [ + 'IPv4 address literal' => ['bob@[192.168.0.1]'], + 'quoted local part' => ['"alice smith"@example.test'], + 'punycode domain' => ['alice@xn--bcher-kva.example'], + 'upper case' => ['ALICE@Example.TEST'], + 'zero width non-joiner' => ["\u{0639}\u{0644}\u{06CC}\u{200C}\u{0631}\u{0636}\u{0627}@example.test"], + ]; + } + + /** + * @dataProvider provideSenders + */ + public function testStoresTheFormSendersAreComparedWith(string $address): void { + $sender = AddressList::parse($address)->first(); + + $normalized = $this->normalizer->normalize($address, AddressEntryType::Individual); + + $this->assertSame($sender?->getEmail(), $normalized); + } + + /** + * @dataProvider provideSenders + */ + public function testKeepsTheStoredFormOfASender(string $address): void { + $stored = $this->normalizer->normalize($address, AddressEntryType::Individual); + + $normalized = $this->normalizer->normalize($stored, AddressEntryType::Individual); + + $this->assertSame($stored, $normalized); + } + + private static function internationalizedDomain(int $octets): string { + // 57 "ü" (2 octets each) are the most that fit a 63-character punycode label + $label = str_repeat('ü', 57); + $domain = "$label.$label.example"; + return str_repeat('a', $octets - strlen($domain) - 1) . ".$domain"; + } +} diff --git a/tests/Unit/Service/InternalAddressServiceTest.php b/tests/Unit/Service/InternalAddressServiceTest.php index 9aba1519df..8c84908593 100644 --- a/tests/Unit/Service/InternalAddressServiceTest.php +++ b/tests/Unit/Service/InternalAddressServiceTest.php @@ -10,27 +10,34 @@ namespace OCA\Mail\Tests\Unit\Service; use ChristophWurst\Nextcloud\Testing\TestCase; +use OCA\Mail\Db\AddressEntryType; use OCA\Mail\Db\InternalAddress; use OCA\Mail\Db\InternalAddressMapper; +use OCA\Mail\Exception\InvalidAddressException; +use OCA\Mail\Service\AddressEntryNormalizer; use OCA\Mail\Service\InternalAddressService; use OCP\DB\Exception; use PHPUnit\Framework\MockObject\MockObject; class InternalAddressServiceTest extends TestCase { private InternalAddressMapper&MockObject $mapper; + private AddressEntryNormalizer&MockObject $normalizer; private InternalAddressService $service; protected function setUp(): void { parent::setUp(); $this->mapper = $this->createMock(InternalAddressMapper::class); + $this->normalizer = $this->createMock(AddressEntryNormalizer::class); $this->service = new InternalAddressService( - $this->mapper + $this->mapper, + $this->normalizer ); } public function testAddAlreadyInternal(): void { + $this->normalizer->method('normalize')->willReturnArgument(0); $this->mapper->expects($this->once()) ->method('exists') ->with('alice', 'bob@example.test') @@ -46,6 +53,7 @@ public function testAddAlreadyInternal(): void { } public function testAddNew(): void { + $this->normalizer->method('normalize')->willReturnArgument(0); $created = new InternalAddress(); $created->setUserId('alice'); $created->setAddress('example.test'); @@ -69,6 +77,7 @@ public function testAddNew(): void { } public function testAddAlreadyStoredDomain(): void { + $this->normalizer->method('normalize')->willReturnArgument(0); $uniqueViolation = $this->createMock(Exception::class); $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); $this->mapper->expects($this->once()) @@ -88,6 +97,7 @@ public function testAddAlreadyStoredDomain(): void { } public function testAddRethrowsOtherDatabaseErrors(): void { + $this->normalizer->method('normalize')->willReturnArgument(0); $databaseError = $this->createMock(Exception::class); $databaseError->method('getReason')->willReturn(Exception::REASON_CONNECTION_LOST); $this->mapper->method('exists') @@ -100,7 +110,84 @@ public function testAddRethrowsOtherDatabaseErrors(): void { $this->service->add('alice', 'example.test', 'domain'); } + public function testAddRejectsAnInvalidEntry(): void { + $this->normalizer->expects($this->once()) + ->method('normalize') + ->with('not valid', AddressEntryType::Domain) + ->willThrowException(new InvalidAddressException('Invalid domain')); + $this->mapper->expects($this->never()) + ->method('exists'); + $this->mapper->expects($this->never()) + ->method('create'); + + $this->expectException(InvalidAddressException::class); + + $this->service->add('alice', 'not valid', 'domain'); + } + + public function testAddRejectsAnUnknownType(): void { + $this->normalizer->expects($this->never()) + ->method('normalize'); + $this->mapper->expects($this->never()) + ->method('create'); + + $this->expectException(InvalidAddressException::class); + $this->expectExceptionMessage('Invalid type'); + + $this->service->add('alice', 'example.test', 'group'); + } + + public static function provideEntries(): array { + return [ + 'individual' => [AddressEntryType::Individual, 'alice@example.test'], + 'domain' => [AddressEntryType::Domain, 'example.test'], + ]; + } + + /** + * @dataProvider provideEntries + */ + public function testAddNormalizesTheValueAsItsType(AddressEntryType $type, string $value): void { + $this->normalizer->expects($this->once()) + ->method('normalize') + ->with($value, $type) + ->willReturnArgument(0); + $this->mapper->method('exists') + ->willReturn(true); + + $this->service->add('alice', $value, $type->value); + } + + public function testAddStoresTheNormalizedValue(): void { + $stored = new InternalAddress(); + $stored->setUserId('alice'); + $stored->setAddress('example.test'); + $stored->setType('domain'); + $this->normalizer->expects($this->once()) + ->method('normalize') + ->with('Example.TEST', AddressEntryType::Domain) + ->willReturn('example.test'); + $this->mapper->expects($this->once()) + ->method('exists') + ->with('alice', 'example.test') + ->willReturn(false); + $this->mapper->expects($this->once()) + ->method('create') + ->with('alice', 'example.test', 'domain') + ->willReturn(1); + $this->mapper->expects($this->once()) + ->method('find') + ->with('alice', 'example.test') + ->willReturn($stored); + + $internalAddress = $this->service->add('alice', 'Example.TEST', 'domain'); + + $this->assertSame($stored, $internalAddress); + } + public function testRemove(): void { + $this->normalizer->expects($this->never()) + ->method('normalize'); $this->mapper->expects($this->never()) ->method('exists'); $this->mapper->expects($this->never()) @@ -113,4 +200,43 @@ public function testRemove(): void { $this->assertNull($internalAddress); } + + public function testRemoveRemovesTheNormalizedEntryToo(): void { + $this->normalizer->method('tryNormalize') + ->with('xn--bcher-kva.example', 'domain') + ->willReturn('bücher.example'); + $removed = []; + $this->mapper->expects($this->exactly(2)) + ->method('remove') + ->willReturnCallback(function (string $uid, string $address, string $type) use (&$removed): void { + $removed[] = ['uid' => $uid, 'address' => $address, 'type' => $type]; + }); + + $this->service->add('alice', 'xn--bcher-kva.example', 'domain', false); + + $this->assertEqualsCanonicalizing([ + ['uid' => 'alice', 'address' => 'xn--bcher-kva.example', 'type' => 'domain'], + ['uid' => 'alice', 'address' => 'bücher.example', 'type' => 'domain'], + ], $removed); + } + + public function testRemoveOfAStoredFormRemovesItOnce(): void { + $this->normalizer->method('tryNormalize') + ->willReturnArgument(0); + $this->mapper->expects($this->once()) + ->method('remove') + ->with('alice', 'bücher.example', 'domain'); + + $this->service->add('alice', 'bücher.example', 'domain', false); + } + + public function testRemoveAcceptsAnUnknownType(): void { + $this->mapper->expects($this->once()) + ->method('remove') + ->with('alice', 'example.test', 'group'); + + $internalAddress = $this->service->add('alice', 'example.test', 'group', false); + + $this->assertNull($internalAddress); + } } diff --git a/tests/Unit/Service/TrustedSenderServiceTest.php b/tests/Unit/Service/TrustedSenderServiceTest.php index 044d535489..afc7b0f947 100644 --- a/tests/Unit/Service/TrustedSenderServiceTest.php +++ b/tests/Unit/Service/TrustedSenderServiceTest.php @@ -10,7 +10,10 @@ namespace OCA\Mail\Tests\Unit\Service; use ChristophWurst\Nextcloud\Testing\TestCase; +use OCA\Mail\Db\AddressEntryType; use OCA\Mail\Db\TrustedSenderMapper; +use OCA\Mail\Exception\InvalidAddressException; +use OCA\Mail\Service\AddressEntryNormalizer; use OCA\Mail\Service\TrustedSenderService; use OCP\DB\Exception; use PHPUnit\Framework\MockObject\MockObject; @@ -19,14 +22,18 @@ class TrustedSenderServiceTest extends TestCase { /** @var TrustedSenderMapper|MockObject */ private $mapper; + private AddressEntryNormalizer&MockObject $normalizer; + /** @var TrustedSenderService */ private $service; protected function setUp(): void { $this->mapper = $this->createMock(TrustedSenderMapper::class); + $this->normalizer = $this->createMock(AddressEntryNormalizer::class); $this->service = new TrustedSenderService( - $this->mapper + $this->mapper, + $this->normalizer ); } @@ -65,6 +72,7 @@ public function testIsNotTrusted(): void { public function testTrustAlreadyTrusted(): void { $uid = 'greta'; $email = 'christoph@next.cloud'; + $this->normalizer->method('normalize')->willReturnArgument(0); $this->mapper->expects($this->once()) ->method('exists') ->with($uid, $email) @@ -84,6 +92,7 @@ public function testTrustAlreadyTrusted(): void { public function testTrustNew(): void { $uid = 'greta'; $email = 'christoph@next.cloud'; + $this->normalizer->method('normalize')->willReturnArgument(0); $this->mapper->expects($this->once()) ->method('exists') ->with($uid, $email) @@ -102,6 +111,7 @@ public function testTrustNew(): void { public function testTrustAlreadyStoredDomain(): void { $uid = 'alice'; $domain = 'example.test'; + $this->normalizer->method('normalize')->willReturnArgument(0); $uniqueViolation = $this->createMock(Exception::class); $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); $this->mapper->expects($this->once()) @@ -123,6 +133,7 @@ public function testTrustAlreadyStoredDomain(): void { public function testTrustRethrowsOtherDatabaseErrors(): void { $uid = 'alice'; $domain = 'example.test'; + $this->normalizer->method('normalize')->willReturnArgument(0); $databaseError = $this->createMock(Exception::class); $databaseError->method('getReason')->willReturn(Exception::REASON_CONNECTION_LOST); $this->mapper->method('exists') @@ -139,9 +150,75 @@ public function testTrustRethrowsOtherDatabaseErrors(): void { ); } + public function testTrustRejectsAnInvalidEntry(): void { + $this->normalizer->expects($this->once()) + ->method('normalize') + ->with('not valid', AddressEntryType::Domain) + ->willThrowException(new InvalidAddressException('Invalid domain')); + $this->mapper->expects($this->never()) + ->method('exists'); + $this->mapper->expects($this->never()) + ->method('create'); + + $this->expectException(InvalidAddressException::class); + + $this->service->trust('alice', 'not valid', 'domain'); + } + + public function testTrustRejectsAnUnknownType(): void { + $this->normalizer->expects($this->never()) + ->method('normalize'); + $this->mapper->expects($this->never()) + ->method('create'); + + $this->expectException(InvalidAddressException::class); + $this->expectExceptionMessage('Invalid type'); + + $this->service->trust('alice', 'example.test', 'group'); + } + + public static function provideEntries(): array { + return [ + 'individual' => [AddressEntryType::Individual, 'alice@example.test'], + 'domain' => [AddressEntryType::Domain, 'example.test'], + ]; + } + + /** + * @dataProvider provideEntries + */ + public function testTrustNormalizesTheValueAsItsType(AddressEntryType $type, string $value): void { + $this->normalizer->expects($this->once()) + ->method('normalize') + ->with($value, $type) + ->willReturnArgument(0); + $this->mapper->method('exists') + ->willReturn(true); + + $this->service->trust('alice', $value, $type->value); + } + + public function testTrustStoresTheNormalizedValue(): void { + $this->normalizer->expects($this->once()) + ->method('normalize') + ->with('Example.TEST', AddressEntryType::Domain) + ->willReturn('example.test'); + $this->mapper->expects($this->once()) + ->method('exists') + ->with('alice', 'example.test') + ->willReturn(false); + $this->mapper->expects($this->once()) + ->method('create') + ->with('alice', 'example.test', 'domain'); + + $this->service->trust('alice', 'Example.TEST', 'domain'); + } + public function testRemoveTrust(): void { $uid = 'greta'; $email = 'christoph@next.cloud'; + $this->normalizer->expects($this->never()) + ->method('normalize'); $this->mapper->expects($this->never()) ->method('exists'); $this->mapper->expects($this->never()) @@ -157,4 +234,41 @@ public function testRemoveTrust(): void { false ); } + + public function testRemoveTrustRemovesTheNormalizedEntryToo(): void { + $this->normalizer->method('tryNormalize') + ->with('xn--bcher-kva.example', 'domain') + ->willReturn('bücher.example'); + $removed = []; + $this->mapper->expects($this->exactly(2)) + ->method('remove') + ->willReturnCallback(function (string $uid, string $email, string $type) use (&$removed): void { + $removed[] = ['uid' => $uid, 'email' => $email, 'type' => $type]; + }); + + $this->service->trust('alice', 'xn--bcher-kva.example', 'domain', false); + + $this->assertEqualsCanonicalizing([ + ['uid' => 'alice', 'email' => 'xn--bcher-kva.example', 'type' => 'domain'], + ['uid' => 'alice', 'email' => 'bücher.example', 'type' => 'domain'], + ], $removed); + } + + public function testRemoveTrustOfAStoredFormRemovesItOnce(): void { + $this->normalizer->method('tryNormalize') + ->willReturnArgument(0); + $this->mapper->expects($this->once()) + ->method('remove') + ->with('alice', 'bücher.example', 'domain'); + + $this->service->trust('alice', 'bücher.example', 'domain', false); + } + + public function testRemoveTrustAcceptsAnUnknownType(): void { + $this->mapper->expects($this->once()) + ->method('remove') + ->with('alice', 'example.test', 'group'); + + $this->service->trust('alice', 'example.test', 'group', false); + } } diff --git a/tests/Unit/UserMigration/Service/InternalAddressesMigrationServiceTest.php b/tests/Unit/UserMigration/Service/InternalAddressesMigrationServiceTest.php index fe9e9ce5ea..eaf52fb4a1 100644 --- a/tests/Unit/UserMigration/Service/InternalAddressesMigrationServiceTest.php +++ b/tests/Unit/UserMigration/Service/InternalAddressesMigrationServiceTest.php @@ -12,6 +12,7 @@ use ChristophWurst\Nextcloud\Testing\ServiceMockObject; use ChristophWurst\Nextcloud\Testing\TestCase; use OCA\Mail\Db\InternalAddress; +use OCA\Mail\Exception\InvalidAddressException; use OCA\Mail\Tests\Unit\UserMigration\RecordsConsoleOutput; use OCA\Mail\UserMigration\Service\InternalAddressesMigrationService; use OCP\IUser; @@ -193,4 +194,30 @@ public function testImportSkipsOnlyTheMalformedInternalAddress(): void { $this->assertWritten('An internal address of ' . self::USER_ID . ' is invalid', 0); } + + public function testImportSkipsOnlyTheRejectedInternalAddress(): void { + $this->recordConsoleOutput(); + $this->importSource->method('getFileContents') + ->with(InternalAddressesMigrationService::INTERNAL_ADDRESSES_FILE) + ->willReturn(json_encode([ + ['id' => 1, 'address' => 'not valid', 'uid' => self::USER_ID, 'type' => 'domain'], + ['id' => 2, 'address' => 'alice@example.test', 'uid' => self::USER_ID, 'type' => 'individual'], + ])); + $imported = []; + $this->serviceMock->getParameter('internalAddressService') + ->expects(self::exactly(2)) + ->method('add') + ->willReturnCallback(static function (string $uid, string $address) use (&$imported): ?InternalAddress { + if ($address === 'not valid') { + throw new InvalidAddressException('Invalid domain'); + } + $imported[] = $address; + return null; + }); + + $this->migrationService->importInternalAddresses($this->user, $this->importSource, $this->output); + + $this->assertSame(['alice@example.test'], $imported); + $this->assertWritten('An internal address of ' . self::USER_ID . ' is invalid', 0); + } } diff --git a/tests/Unit/UserMigration/Service/TrustedSendersMigrationServiceTest.php b/tests/Unit/UserMigration/Service/TrustedSendersMigrationServiceTest.php index b3e3c6d6ae..4a435cfd45 100644 --- a/tests/Unit/UserMigration/Service/TrustedSendersMigrationServiceTest.php +++ b/tests/Unit/UserMigration/Service/TrustedSendersMigrationServiceTest.php @@ -12,6 +12,7 @@ use ChristophWurst\Nextcloud\Testing\ServiceMockObject; use ChristophWurst\Nextcloud\Testing\TestCase; use OCA\Mail\Db\TrustedSender; +use OCA\Mail\Exception\InvalidAddressException; use OCA\Mail\Tests\Unit\UserMigration\RecordsConsoleOutput; use OCA\Mail\UserMigration\Service\TrustedSendersMigrationService; use OCP\IUser; @@ -193,4 +194,29 @@ public function testImportSkipsOnlyTheMalformedTrustedSender(): void { $this->assertWritten('A trusted sender of ' . self::USER_ID . ' is invalid', 0); } + + public function testImportSkipsOnlyTheRejectedTrustedSender(): void { + $this->recordConsoleOutput(); + $this->importSource->method('getFileContents') + ->with(TrustedSendersMigrationService::TRUSTED_SENDERS_FILE) + ->willReturn(json_encode([ + ['email' => 'not valid', 'type' => 'domain'], + ['email' => 'alice@example.test', 'type' => 'individual'], + ])); + $imported = []; + $this->serviceMock->getParameter('trustedSenderService') + ->expects(self::exactly(2)) + ->method('trust') + ->willReturnCallback(static function (string $uid, string $email) use (&$imported): void { + if ($email === 'not valid') { + throw new InvalidAddressException('Invalid domain'); + } + $imported[] = $email; + }); + + $this->migrationService->importTrustedSenders($this->user, $this->importSource, $this->output); + + $this->assertSame(['alice@example.test'], $imported); + $this->assertWritten('A trusted sender of ' . self::USER_ID . ' is invalid', 0); + } } From 2525094494f49e124093b3ff7eb2227a116d1bd2 Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Thu, 5 Nov 2026 17:21:58 +0100 Subject: [PATCH 3/5] fix(settings): save an address even if its domain is listed trust() and add() returned early when isTrusted() or isInternal() matched, and both also match an address through its listed domain. Trusting alice@example.test while example.test is trusted, or marking it as internal while example.test is internal, therefore saved nothing, and removing the domain later dropped the address as well. A unique constraint violation is already caught, so the lookup isn't needed to avoid duplicate entries. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner --- lib/Contracts/IInternalAddressService.php | 2 - lib/Db/InternalAddressMapper.php | 27 -------- lib/Service/InternalAddressService.php | 61 +++++------------- lib/Service/TrustedSenderService.php | 48 +++++--------- .../Db/InternalAddressMapperTest.php | 38 ------------ .../Service/InternalAddressServiceTest.php | 62 ++----------------- .../Unit/Service/TrustedSenderServiceTest.php | 50 +++------------ 7 files changed, 43 insertions(+), 245 deletions(-) diff --git a/lib/Contracts/IInternalAddressService.php b/lib/Contracts/IInternalAddressService.php index d6c1746963..26d2465543 100644 --- a/lib/Contracts/IInternalAddressService.php +++ b/lib/Contracts/IInternalAddressService.php @@ -13,8 +13,6 @@ use OCA\Mail\Exception\InvalidAddressException; interface IInternalAddressService { - public function isInternal(string $uid, string $address): bool; - /** * @throws InvalidAddressException if the $address to add or its $type is invalid */ diff --git a/lib/Db/InternalAddressMapper.php b/lib/Db/InternalAddressMapper.php index a4f678b6da..47014362f0 100644 --- a/lib/Db/InternalAddressMapper.php +++ b/lib/Db/InternalAddressMapper.php @@ -21,33 +21,6 @@ public function __construct(IDBConnection $db) { parent::__construct($db, 'mail_internal_address'); } - public function exists(string $uid, string $address): bool { - - $emailObject = new \Horde_Mail_Rfc822_Address($address); - $host = $emailObject->host; - $qb = $this->db->getQueryBuilder(); - - $select = $qb->select('*') - ->from($this->getTableName()) - ->where( - $qb->expr()->orX( - $qb->expr()->andX( - $qb->expr()->eq('address', $qb->createNamedParameter($address)), - $qb->expr()->eq('type', $qb->createNamedParameter(AddressEntryType::Individual->value)) - ), - $qb->expr()->andX( - $qb->expr()->eq('address', $qb->createNamedParameter($host)), - $qb->expr()->eq('type', $qb->createNamedParameter(AddressEntryType::Domain->value)) - ) - ), - $qb->expr()->eq('user_id', $qb->createNamedParameter($uid)) - ); - - $rows = $this->findEntities($select); - - return $rows !== []; - } - public function create(string $uid, string $address, string $type): int { $address = InternalAddress::fromParams([ diff --git a/lib/Service/InternalAddressService.php b/lib/Service/InternalAddressService.php index 0566bc1093..2e589f0e61 100644 --- a/lib/Service/InternalAddressService.php +++ b/lib/Service/InternalAddressService.php @@ -23,59 +23,32 @@ public function __construct( ) { } - #[\Override] - public function isInternal(string $uid, string $address): bool { - return $this->mapper->exists( - $uid, - $address - ); - } - /** * @throws InvalidAddressException */ #[\Override] public function add(string $uid, string $address, string $type, ?bool $trust = true): ?InternalAddress { - if ($trust) { - $entryType = AddressEntryType::tryFrom($type) ?? throw new InvalidAddressException('Invalid type'); - $address = $this->normalizer->normalize($address, $entryType); - if ($this->isInternal($uid, $address)) { - // Nothing to do - return null; + if (!$trust) { + // Valid values are stored normalized, older entries as they were entered + foreach (array_unique([$address, $this->normalizer->tryNormalize($address, $type) ?? $address]) as $storedAddress) { + $this->mapper->remove($uid, $storedAddress, $type); } + return null; + } - try { - $this->mapper->create( - $uid, - $address, - $type - ); - } catch (Exception $exception) { - if ($exception->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - throw $exception; - } - - // isInternal() doesn't match a domain itself, and requests can race - return null; - } - return $this->getInternalAddress($uid, $address); - } else { - $this->mapper->remove( - $uid, - $address, - $type - ); - // Valid values are stored normalized, older entries as they were entered - $normalized = $this->normalizer->tryNormalize($address, $type); - if ($normalized !== null && $normalized !== $address) { - $this->mapper->remove( - $uid, - $normalized, - $type - ); + $entryType = AddressEntryType::tryFrom($type) ?? throw new InvalidAddressException('Invalid type'); + $address = $this->normalizer->normalize($address, $entryType); + try { + $this->mapper->create($uid, $address, $entryType->value); + } catch (Exception $exception) { + if ($exception->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $exception; } + + // The address is listed already, e.g. added again from an outdated list or by two requests at once + return null; } - return null; + return $this->getInternalAddress($uid, $address); } #[\Override] diff --git a/lib/Service/TrustedSenderService.php b/lib/Service/TrustedSenderService.php index 7f816b77d8..a64a540307 100644 --- a/lib/Service/TrustedSenderService.php +++ b/lib/Service/TrustedSenderService.php @@ -53,42 +53,24 @@ public function isSenderTrusted(string $uid, Message $message): bool { */ #[\Override] public function trust(string $uid, string $email, string $type, ?bool $trust = true): void { - if ($trust) { - $entryType = AddressEntryType::tryFrom($type) ?? throw new InvalidAddressException('Invalid type'); - $email = $this->normalizer->normalize($email, $entryType); - if ($this->isTrusted($uid, $email)) { - // Nothing to do - return; + if (!$trust) { + // Valid values are stored normalized, older entries as they were entered + foreach (array_unique([$email, $this->normalizer->tryNormalize($email, $type) ?? $email]) as $storedEmail) { + $this->mapper->remove($uid, $storedEmail, $type); } + return; + } - try { - $this->mapper->create( - $uid, - $email, - $type - ); - } catch (Exception $exception) { - if ($exception->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - throw $exception; - } - - // isTrusted() doesn't match a domain itself, and requests can race - } - } else { - $this->mapper->remove( - $uid, - $email, - $type - ); - // Valid values are stored normalized, older entries as they were entered - $normalized = $this->normalizer->tryNormalize($email, $type); - if ($normalized !== null && $normalized !== $email) { - $this->mapper->remove( - $uid, - $normalized, - $type - ); + $entryType = AddressEntryType::tryFrom($type) ?? throw new InvalidAddressException('Invalid type'); + $email = $this->normalizer->normalize($email, $entryType); + try { + $this->mapper->create($uid, $email, $entryType->value); + } catch (Exception $exception) { + if ($exception->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $exception; } + + // The sender is listed already, e.g. trusted again from an outdated list or by two requests at once } } diff --git a/tests/Integration/Db/InternalAddressMapperTest.php b/tests/Integration/Db/InternalAddressMapperTest.php index b1488e13d2..d592825af3 100644 --- a/tests/Integration/Db/InternalAddressMapperTest.php +++ b/tests/Integration/Db/InternalAddressMapperTest.php @@ -43,44 +43,6 @@ protected function setUp(): void { ); } - public function testDoesntExist(): void { - $exists = $this->mapper->exists($this->user->getUID(), 'hamza@next.cloud'); - - $this->assertFalse($exists); - } - - public function testIndividualExists(): void { - $uid = $this->user->getUID(); - $qb = $this->db->getQueryBuilder(); - $qb->insert('mail_internal_address') - ->values([ - 'user_id' => $qb->createNamedParameter($uid), - 'address' => $qb->createNamedParameter('hamza@next.cloud'), - 'type' => $qb->createNamedParameter('individual') - ]) - ->executeStatement(); - - $exists = $this->mapper->exists($uid, 'hamza@next.cloud'); - - $this->assertTrue($exists); - } - - public function testDomainExists(): void { - $uid = $this->user->getUID(); - $qb = $this->db->getQueryBuilder(); - $qb->insert('mail_internal_address') - ->values([ - 'user_id' => $qb->createNamedParameter($uid), - 'address' => $qb->createNamedParameter('next.cloud'), - 'type' => $qb->createNamedParameter('domain'), - ]) - ->executeStatement(); - - $exists = $this->mapper->exists($uid, 'hamza@next.cloud'); - - $this->assertTrue($exists); - } - public function testCreateIndividual(): void { $uid = $this->user->getUID(); $this->mapper->create( diff --git a/tests/Unit/Service/InternalAddressServiceTest.php b/tests/Unit/Service/InternalAddressServiceTest.php index 8c84908593..a63247a05a 100644 --- a/tests/Unit/Service/InternalAddressServiceTest.php +++ b/tests/Unit/Service/InternalAddressServiceTest.php @@ -37,61 +37,19 @@ protected function setUp(): void { } public function testAddAlreadyInternal(): void { - $this->normalizer->method('normalize')->willReturnArgument(0); - $this->mapper->expects($this->once()) - ->method('exists') - ->with('alice', 'bob@example.test') - ->willReturn(true); - $this->mapper->expects($this->never()) - ->method('create'); - $this->mapper->expects($this->never()) - ->method('remove'); - - $internalAddress = $this->service->add('alice', 'bob@example.test', 'individual'); - - $this->assertNull($internalAddress); - } - - public function testAddNew(): void { - $this->normalizer->method('normalize')->willReturnArgument(0); - $created = new InternalAddress(); - $created->setUserId('alice'); - $created->setAddress('example.test'); - $created->setType('domain'); - $this->mapper->expects($this->once()) - ->method('exists') - ->with('alice', 'example.test') - ->willReturn(false); - $this->mapper->expects($this->once()) - ->method('create') - ->with('alice', 'example.test', 'domain') - ->willReturn(1); - $this->mapper->expects($this->once()) - ->method('find') - ->with('alice', 'example.test') - ->willReturn($created); - - $internalAddress = $this->service->add('alice', 'example.test', 'domain'); - - $this->assertSame($created, $internalAddress); - } - - public function testAddAlreadyStoredDomain(): void { $this->normalizer->method('normalize')->willReturnArgument(0); $uniqueViolation = $this->createMock(Exception::class); $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); - $this->mapper->expects($this->once()) - ->method('exists') - ->with('alice', 'example.test') - ->willReturn(false); $this->mapper->expects($this->once()) ->method('create') - ->with('alice', 'example.test', 'domain') + ->with('alice', 'bob@example.test', 'individual') ->willThrowException($uniqueViolation); $this->mapper->expects($this->never()) ->method('find'); + $this->mapper->expects($this->never()) + ->method('remove'); - $internalAddress = $this->service->add('alice', 'example.test', 'domain'); + $internalAddress = $this->service->add('alice', 'bob@example.test', 'individual'); $this->assertNull($internalAddress); } @@ -100,8 +58,6 @@ public function testAddRethrowsOtherDatabaseErrors(): void { $this->normalizer->method('normalize')->willReturnArgument(0); $databaseError = $this->createMock(Exception::class); $databaseError->method('getReason')->willReturn(Exception::REASON_CONNECTION_LOST); - $this->mapper->method('exists') - ->willReturn(false); $this->mapper->method('create') ->willThrowException($databaseError); @@ -115,8 +71,6 @@ public function testAddRejectsAnInvalidEntry(): void { ->method('normalize') ->with('not valid', AddressEntryType::Domain) ->willThrowException(new InvalidAddressException('Invalid domain')); - $this->mapper->expects($this->never()) - ->method('exists'); $this->mapper->expects($this->never()) ->method('create'); @@ -152,8 +106,6 @@ public function testAddNormalizesTheValueAsItsType(AddressEntryType $type, strin ->method('normalize') ->with($value, $type) ->willReturnArgument(0); - $this->mapper->method('exists') - ->willReturn(true); $this->service->add('alice', $value, $type->value); } @@ -167,10 +119,6 @@ public function testAddStoresTheNormalizedValue(): void { ->method('normalize') ->with('Example.TEST', AddressEntryType::Domain) ->willReturn('example.test'); - $this->mapper->expects($this->once()) - ->method('exists') - ->with('alice', 'example.test') - ->willReturn(false); $this->mapper->expects($this->once()) ->method('create') ->with('alice', 'example.test', 'domain') @@ -188,8 +136,6 @@ public function testAddStoresTheNormalizedValue(): void { public function testRemove(): void { $this->normalizer->expects($this->never()) ->method('normalize'); - $this->mapper->expects($this->never()) - ->method('exists'); $this->mapper->expects($this->never()) ->method('create'); $this->mapper->expects($this->once()) diff --git a/tests/Unit/Service/TrustedSenderServiceTest.php b/tests/Unit/Service/TrustedSenderServiceTest.php index afc7b0f947..587c8eb923 100644 --- a/tests/Unit/Service/TrustedSenderServiceTest.php +++ b/tests/Unit/Service/TrustedSenderServiceTest.php @@ -73,12 +73,12 @@ public function testTrustAlreadyTrusted(): void { $uid = 'greta'; $email = 'christoph@next.cloud'; $this->normalizer->method('normalize')->willReturnArgument(0); + $uniqueViolation = $this->createMock(Exception::class); + $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); $this->mapper->expects($this->once()) - ->method('exists') - ->with($uid, $email) - ->willReturn(true); - $this->mapper->expects($this->never()) - ->method('create'); + ->method('create') + ->with($uid, $email, 'individual') + ->willThrowException($uniqueViolation); $this->mapper->expects($this->never()) ->method('remove'); @@ -93,10 +93,8 @@ public function testTrustNew(): void { $uid = 'greta'; $email = 'christoph@next.cloud'; $this->normalizer->method('normalize')->willReturnArgument(0); - $this->mapper->expects($this->once()) - ->method('exists') - ->with($uid, $email) - ->willReturn(false); + $this->mapper->expects($this->never()) + ->method('exists'); $this->mapper->expects($this->once()) ->method('create') ->with($uid, $email); @@ -108,36 +106,12 @@ public function testTrustNew(): void { ); } - public function testTrustAlreadyStoredDomain(): void { - $uid = 'alice'; - $domain = 'example.test'; - $this->normalizer->method('normalize')->willReturnArgument(0); - $uniqueViolation = $this->createMock(Exception::class); - $uniqueViolation->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION); - $this->mapper->expects($this->once()) - ->method('exists') - ->with($uid, $domain) - ->willReturn(false); - $this->mapper->expects($this->once()) - ->method('create') - ->with($uid, $domain, 'domain') - ->willThrowException($uniqueViolation); - - $this->service->trust( - $uid, - $domain, - 'domain' - ); - } - public function testTrustRethrowsOtherDatabaseErrors(): void { $uid = 'alice'; $domain = 'example.test'; $this->normalizer->method('normalize')->willReturnArgument(0); $databaseError = $this->createMock(Exception::class); $databaseError->method('getReason')->willReturn(Exception::REASON_CONNECTION_LOST); - $this->mapper->method('exists') - ->willReturn(false); $this->mapper->method('create') ->willThrowException($databaseError); @@ -155,8 +129,6 @@ public function testTrustRejectsAnInvalidEntry(): void { ->method('normalize') ->with('not valid', AddressEntryType::Domain) ->willThrowException(new InvalidAddressException('Invalid domain')); - $this->mapper->expects($this->never()) - ->method('exists'); $this->mapper->expects($this->never()) ->method('create'); @@ -192,8 +164,6 @@ public function testTrustNormalizesTheValueAsItsType(AddressEntryType $type, str ->method('normalize') ->with($value, $type) ->willReturnArgument(0); - $this->mapper->method('exists') - ->willReturn(true); $this->service->trust('alice', $value, $type->value); } @@ -203,10 +173,6 @@ public function testTrustStoresTheNormalizedValue(): void { ->method('normalize') ->with('Example.TEST', AddressEntryType::Domain) ->willReturn('example.test'); - $this->mapper->expects($this->once()) - ->method('exists') - ->with('alice', 'example.test') - ->willReturn(false); $this->mapper->expects($this->once()) ->method('create') ->with('alice', 'example.test', 'domain'); @@ -219,8 +185,6 @@ public function testRemoveTrust(): void { $email = 'christoph@next.cloud'; $this->normalizer->expects($this->never()) ->method('normalize'); - $this->mapper->expects($this->never()) - ->method('exists'); $this->mapper->expects($this->never()) ->method('create'); $this->mapper->expects($this->once()) From a14d3bb77e811132a036351a37e3362d5c53d49d Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Fri, 6 Nov 2026 11:20:09 +0100 Subject: [PATCH 4/5] fixup! fix(settings): reject invalid addresses and domains Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner --- lib/Contracts/IInternalAddressService.php | 2 +- lib/Contracts/ITrustedSenderService.php | 2 +- lib/Service/AddressEntryNormalizer.php | 3 +-- tests/Unit/Service/AddressEntryNormalizerTest.php | 1 - 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/Contracts/IInternalAddressService.php b/lib/Contracts/IInternalAddressService.php index 26d2465543..5a77ab4dba 100644 --- a/lib/Contracts/IInternalAddressService.php +++ b/lib/Contracts/IInternalAddressService.php @@ -14,7 +14,7 @@ interface IInternalAddressService { /** - * @throws InvalidAddressException if the $address to add or its $type is invalid + * @throws InvalidAddressException */ public function add(string $uid, string $address, string $type, ?bool $trust = true); diff --git a/lib/Contracts/ITrustedSenderService.php b/lib/Contracts/ITrustedSenderService.php index 7dd0117153..2004b123fe 100644 --- a/lib/Contracts/ITrustedSenderService.php +++ b/lib/Contracts/ITrustedSenderService.php @@ -16,7 +16,7 @@ interface ITrustedSenderService { public function isTrusted(string $uid, string $email): bool; /** - * @throws InvalidAddressException if the $email to trust or its $type is invalid + * @throws InvalidAddressException */ public function trust(string $uid, string $email, string $type, ?bool $trust = true); diff --git a/lib/Service/AddressEntryNormalizer.php b/lib/Service/AddressEntryNormalizer.php index b0265d821e..8953b32b87 100644 --- a/lib/Service/AddressEntryNormalizer.php +++ b/lib/Service/AddressEntryNormalizer.php @@ -85,7 +85,7 @@ private function normalizeAddress(string $value): string { } $addresses = $this->parseAddressList($value) - // The stored form of a sender drops the quotes around its local part, e.g. of "alice smith"@example.test + // A sender's stored form drops the quotes around its local part ?? $this->parseAddressList('"' . addcslashes($localPart, '"\\') . '"@' . $domain); $address = $addresses !== null && count($addresses) === 1 ? $addresses->first() : null; if (!($address instanceof Horde_Mail_Rfc822_Address)) { @@ -98,7 +98,6 @@ private function normalizeAddress(string $value): string { private function parseAddressList(string $value): ?Horde_Mail_Rfc822_List { try { - // RFC 5322, with the UTF-8 addresses of RFC 6532 return (new Horde_Mail_Rfc822())->parseAddressList($value, ['validate' => 'eai']); } catch (Horde_Mail_Exception) { return null; diff --git a/tests/Unit/Service/AddressEntryNormalizerTest.php b/tests/Unit/Service/AddressEntryNormalizerTest.php index 93a5e88397..6dd8bacbb5 100644 --- a/tests/Unit/Service/AddressEntryNormalizerTest.php +++ b/tests/Unit/Service/AddressEntryNormalizerTest.php @@ -187,7 +187,6 @@ public function testAcceptsAStoredFormOf254Octets(): void { public static function provideValuesOver254Octets(): array { return [ - // RFC 5321 limits a path to 256 octets including the angle brackets, which leaves 254 'address of 255 octets' => [str_repeat('a', 242) . '@example.test', AddressEntryType::Individual], 'domain of 255 octets' => [self::internationalizedDomain(255), AddressEntryType::Domain], // Each "．" (U+FF0E, 3 octets) becomes ".", so only the value as given is too long From 97af1d5c746adc02fe8a2a8ab3e3f287f6d61796 Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Fri, 6 Nov 2026 11:20:19 +0100 Subject: [PATCH 5/5] fixup! fix(settings): save an address even if its domain is listed Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner --- lib/Service/InternalAddressService.php | 2 +- lib/Service/TrustedSenderService.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Service/InternalAddressService.php b/lib/Service/InternalAddressService.php index 2e589f0e61..a9814f8768 100644 --- a/lib/Service/InternalAddressService.php +++ b/lib/Service/InternalAddressService.php @@ -45,7 +45,7 @@ public function add(string $uid, string $address, string $type, ?bool $trust = t throw $exception; } - // The address is listed already, e.g. added again from an outdated list or by two requests at once + // Already listed, e.g. by a concurrent request return null; } return $this->getInternalAddress($uid, $address); diff --git a/lib/Service/TrustedSenderService.php b/lib/Service/TrustedSenderService.php index a64a540307..dbc0d6d6ea 100644 --- a/lib/Service/TrustedSenderService.php +++ b/lib/Service/TrustedSenderService.php @@ -70,7 +70,7 @@ public function trust(string $uid, string $email, string $type, ?bool $trust = t throw $exception; } - // The sender is listed already, e.g. trusted again from an outdated list or by two requests at once + // Already listed, e.g. by a concurrent request } }