Skip to content

Commit 24b0191

Browse files
SebastianKrupinskiSebastianKrupinski
committed
fix: normalize email addresses before using them
Signed-off-by: SebastianKrupinski <[email protected]>
1 parent 27e1ae5 commit 24b0191

File tree

7 files changed

+148
-14
lines changed

7 files changed

+148
-14
lines changed

‎lib/Address.php‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,21 @@ private function __construct(Horde_Mail_Rfc822_Address $wrapped) {
2929
$this->wrapped = $wrapped;
3030
}
3131

32-
public static function fromHorde(Horde_Mail_Rfc822_Address $horde): self {
32+
public static function fromHorde(Horde_Mail_Rfc822_Address $horde, bool $normalize = false): self {
33+
if ($normalize) {
34+
return self::fromRaw($horde->personal, $horde->bare_address, $normalize);
35+
}
3336
return new self($horde);
3437
}
3538

36-
public static function fromRaw(string $label, string $email): self {
37-
$wrapped = new Horde_Mail_Rfc822_Address($email);
39+
public static function fromRaw(?string $label, string $email, bool $normalize = false): self {
40+
if ($normalize) {
41+
$wrapped = new Horde_Mail_Rfc822_Address(self::normalizeAddress($email));
42+
} else {
43+
$wrapped = new Horde_Mail_Rfc822_Address($email);
44+
}
3845
// If no label is set we use the email
39-
if ($label !== $email) {
46+
if ($label !== null && $label !== $email) {
4047
$wrapped->personal = $label;
4148
}
4249
return new self($wrapped);
@@ -117,4 +124,13 @@ public function equals($object): bool {
117124
return $this->getEmail() === $object->getEmail()
118125
&& $this->getLabel() === $object->getLabel();
119126
}
127+
128+
private static function normalizeAddress(string $address): string {
129+
// remove single quotes and whitespace the might exist
130+
// Examples:
131+
// [email protected]
132+
// '[email protected]'
133+
// ' [email protected]'
134+
return strtolower(trim(trim($address, "'")));
135+
}
120136
}

‎lib/AddressList.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,9 @@ public static function parse($str) {
4747
* @param Horde_Mail_Rfc822_List $hordeList
4848
* @return AddressList
4949
*/
50-
public static function fromHorde(Horde_Mail_Rfc822_List $hordeList) {
51-
$addresses = array_map(static function (Horde_Mail_Rfc822_Address $addr) {
52-
return Address::fromHorde($addr);
50+
public static function fromHorde(Horde_Mail_Rfc822_List $hordeList, bool $normalize = false): self {
51+
$addresses = array_map(static function (Horde_Mail_Rfc822_Address $addr) use ($normalize) {
52+
return Address::fromHorde($addr, $normalize);
5353
}, array_filter(iterator_to_array($hordeList), static function (Horde_Mail_Rfc822_Object $obj) {
5454
// TODO: how to handle non-addresses? This doesn't seem right …
5555
return $obj instanceof Horde_Mail_Rfc822_Address;

‎lib/BackgroundJob/RepairRecipients.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Mail\BackgroundJob;
11+
12+
use OCP\AppFramework\Utility\ITimeFactory;
13+
use OCP\BackgroundJob\IJobList;
14+
use OCP\BackgroundJob\TimedJob;
15+
use OCP\DB\QueryBuilder\IQueryBuilder;
16+
use OCP\IDBConnection;
17+
18+
class RepairRecipients extends TimedJob {
19+
20+
public function __construct(
21+
protected ITimeFactory $time,
22+
private IDBConnection $db,
23+
private IJobList $jobService,
24+
) {
25+
parent::__construct($time);
26+
$this->setInterval(300);
27+
}
28+
29+
protected function run($argument): void {
30+
// fetch all quoted emails
31+
$select = $this->db->getQueryBuilder();
32+
$select->select('id', 'email')
33+
->from('mail_recipients')
34+
->where(
35+
$select->expr()->like('email', $select->createNamedParameter('\'%\'', IQueryBuilder::PARAM_STR))
36+
)
37+
->setMaxResults(1000);
38+
$recipients = $select->executeQuery()->fetchAll();
39+
// update emails
40+
$update = $this->db->getQueryBuilder();
41+
$update->update('mail_recipients')
42+
->set('email', $update->createParameter('email'))
43+
->where($update->expr()->in('id', $update->createParameter('id'), IQueryBuilder::PARAM_STR));
44+
foreach ($recipients as $recipient) {
45+
$id = $recipient['id'];
46+
$email = $recipient['email'];
47+
$email = trim(str_replace('\'', '', (string)$email));
48+
$update->setParameter('id', $id, IQueryBuilder::PARAM_STR);
49+
$update->setParameter('email', $email, IQueryBuilder::PARAM_STR);
50+
$update->executeStatement();
51+
}
52+
// remove job depending on the result
53+
if ($recipients === []) {
54+
$this->jobService->remove(RepairRecipients::class);
55+
}
56+
}
57+
58+
}

‎lib/IMAP/ImapMessageFetcher.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -250,11 +250,11 @@ public function fetchMessage(?Horde_Imap_Client_Data_Fetch $fetch = null): IMAPM
250250
$this->uid,
251251
$envelope->message_id,
252252
$fetch->getFlags(),
253-
AddressList::fromHorde($envelope->from),
254-
AddressList::fromHorde($envelope->to),
255-
AddressList::fromHorde($envelope->cc),
256-
AddressList::fromHorde($envelope->bcc),
257-
AddressList::fromHorde($envelope->reply_to),
253+
AddressList::fromHorde($envelope->from, true),
254+
AddressList::fromHorde($envelope->to, true),
255+
AddressList::fromHorde($envelope->cc, true),
256+
AddressList::fromHorde($envelope->bcc, true),
257+
AddressList::fromHorde($envelope->reply_to, true),
258258
$this->decodeSubject($envelope),
259259
$this->plainMessage,
260260
$this->htmlMessage,

‎lib/Migration/Version5000Date20250507000000.php‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Mail\Migration;
11+
12+
use Closure;
13+
use OCP\BackgroundJob\IJobList;
14+
use OCP\Migration\IOutput;
15+
use OCP\Migration\SimpleMigrationStep;
16+
17+
class Version5000Date20250507000000 extends SimpleMigrationStep {
18+
19+
public function __construct(
20+
private IJobList $jobService,
21+
) {
22+
}
23+
24+
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
25+
$this->jobService->add(\OCA\Mail\BackgroundJob\RepairRecipients::class);
26+
}
27+
28+
}

‎lib/Service/PhishingDetection/PhishingDetectionService.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public function checkHeadersForPhishing(Horde_Mime_Headers $headers, bool $hasHt
3333
$customEmail = null;
3434
$fromHeader = $headers->getHeader('From');
3535
if ($fromHeader instanceof Horde_Mime_Headers_Element_Address) {
36-
$firstAddr = AddressList::fromHorde($fromHeader->getAddressList(true))?->first();
36+
$firstAddr = AddressList::fromHorde($fromHeader->getAddressList(true), true)->first();
3737
$fromFN = $firstAddr?->getLabel();
3838
$fromEmail = $firstAddr?->getEmail();
3939
$customEmail = $firstAddr?->getCustomEmail();
@@ -45,7 +45,7 @@ public function checkHeadersForPhishing(Horde_Mime_Headers $headers, bool $hasHt
4545
if ($replyToHeader instanceof Horde_Mime_Headers_Element_Address) {
4646
$replyToAddrs = $replyToHeader->getAddressList(true);
4747
if (isset($replyToAddrs)) {
48-
$replyToEmail = AddressList::fromHorde($replyToAddrs)->first()?->getEmail();
48+
$replyToEmail = AddressList::fromHorde($replyToAddrs, true)->first()?->getEmail();
4949
}
5050
}
5151

‎tests/AddressTest.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,36 @@ public function testDoesNotEqualBecauseDifferentLabel() {
6868

6969
$this->assertFalse($equals);
7070
}
71+
72+
public function testNormalizedWithSingleQuotes() {
73+
$address = Address::fromRaw(null, "'[email protected]'", true)->toHorde();
74+
$this->assertEquals('[email protected]', $address->bare_address);
75+
76+
$address = Address::fromHorde(new Horde_Mail_Rfc822_Address("'[email protected]'"), true)->toHorde();
77+
$this->assertEquals('[email protected]', $address->bare_address);
78+
}
79+
80+
public function testUnnormalizedWithSingleQuotes() {
81+
$address = Address::fromRaw(null, "'[email protected]'", false)->toHorde();
82+
$this->assertEquals("'[email protected]'", $address->bare_address);
83+
84+
$address = Address::fromHorde(new Horde_Mail_Rfc822_Address("'[email protected]'"), false)->toHorde();
85+
$this->assertEquals("'[email protected]'", $address->bare_address);
86+
}
87+
88+
public function testNormalizedWithUpperCaseLetters() {
89+
$address = Address::fromRaw(null, '[email protected]', true)->toHorde();
90+
$this->assertEquals('[email protected]', $address->bare_address);
91+
92+
$address = Address::fromHorde(new Horde_Mail_Rfc822_Address('[email protected]'), true)->toHorde();
93+
$this->assertEquals('[email protected]', $address->bare_address);
94+
}
95+
96+
public function testUnnormalizedWithUpperCaseLetters() {
97+
$address = Address::fromRaw(null, '[email protected]', false)->toHorde();
98+
$this->assertEquals('[email protected]', $address->bare_address);
99+
100+
$address = Address::fromHorde(new Horde_Mail_Rfc822_Address('[email protected]'), false)->toHorde();
101+
$this->assertEquals('[email protected]', $address->bare_address);
102+
}
71103
}

0 commit comments

Comments
 (0)