Skip to content

Commit eabe3a4

Browse files
author
epriestley
committed
Begin improving the soundness of received mail
Summary: We/I broke a couple of things here recently (see D5911) and are doing some work here in general (see D5912, etc.). Generally, this code is pretty oldschool and not especially well architected for modern application-oriented Phabricator. It hardcodes a lot of stuff which should be applications' responsibilites. Take the first steps toward making it more solid to reduce the risk here. In particular: - Factor out the "self mail" and "duplicate mail" checks and add unit tests. - Make Message-ID hash handling automatic. Test Plan: Ran unit tests. Reviewers: btrahan, chad Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D5915
1 parent 99f648e commit eabe3a4

10 files changed

+180
-44
lines changed

‎resources/sql/patches/20130513.receviedmailstatus.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
ALTER TABLE {$NAMESPACE}_metamta.metamta_receivedmail
2+
ADD status VARCHAR(32) NOT NULL;

‎scripts/mail/mail_handler.php

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,6 @@
3434
'text' => $text_body,
3535
'html' => $parser->getMessageBody('html'),
3636
));
37-
$received->setMessageIDHash(
38-
PhabricatorHash::digestForIndex($received->getMessageID())
39-
);
4037

4138
$attachments = array();
4239
foreach ($parser->getAttachments() as $attachment) {

‎src/__phutil_library_map__.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,6 +653,7 @@
653653
'ManiphestView' => 'applications/maniphest/view/ManiphestView.php',
654654
'MetaMTAConstants' => 'applications/metamta/constants/MetaMTAConstants.php',
655655
'MetaMTANotificationType' => 'applications/metamta/constants/MetaMTANotificationType.php',
656+
'MetaMTAReceivedMailStatus' => 'applications/metamta/constants/MetaMTAReceivedMailStatus.php',
656657
'ObjectHandleLoader' => 'applications/phid/handle/ObjectHandleLoader.php',
657658
'OwnersPackageReplyHandler' => 'applications/owners/OwnersPackageReplyHandler.php',
658659
'PHUI' => 'view/phui/PHUI.php',
@@ -1107,6 +1108,8 @@
11071108
'PhabricatorMetaMTAReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTAReceiveController.php',
11081109
'PhabricatorMetaMTAReceivedListController' => 'applications/metamta/controller/PhabricatorMetaMTAReceivedListController.php',
11091110
'PhabricatorMetaMTAReceivedMail' => 'applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php',
1111+
'PhabricatorMetaMTAReceivedMailProcessingException' => 'applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php',
1112+
'PhabricatorMetaMTAReceivedMailTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php',
11101113
'PhabricatorMetaMTASendController' => 'applications/metamta/controller/PhabricatorMetaMTASendController.php',
11111114
'PhabricatorMetaMTASendGridReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php',
11121115
'PhabricatorMetaMTAViewController' => 'applications/metamta/controller/PhabricatorMetaMTAViewController.php',
@@ -2391,6 +2394,7 @@
23912394
'ManiphestTransactionType' => 'ManiphestConstants',
23922395
'ManiphestView' => 'AphrontView',
23932396
'MetaMTANotificationType' => 'MetaMTAConstants',
2397+
'MetaMTAReceivedMailStatus' => 'MetaMTAConstants',
23942398
'OwnersPackageReplyHandler' => 'PhabricatorMailReplyHandler',
23952399
'PHUIBoxExample' => 'PhabricatorUIExample',
23962400
'PHUIBoxView' => 'AphrontTagView',
@@ -2832,6 +2836,8 @@
28322836
'PhabricatorMetaMTAReceiveController' => 'PhabricatorMetaMTAController',
28332837
'PhabricatorMetaMTAReceivedListController' => 'PhabricatorMetaMTAController',
28342838
'PhabricatorMetaMTAReceivedMail' => 'PhabricatorMetaMTADAO',
2839+
'PhabricatorMetaMTAReceivedMailProcessingException' => 'Exception',
2840+
'PhabricatorMetaMTAReceivedMailTestCase' => 'PhabricatorTestCase',
28352841
'PhabricatorMetaMTASendController' => 'PhabricatorMetaMTAController',
28362842
'PhabricatorMetaMTASendGridReceiveController' => 'PhabricatorMetaMTAController',
28372843
'PhabricatorMetaMTAViewController' => 'PhabricatorMetaMTAController',

‎src/applications/metamta/constants/MetaMTAReceivedMailStatus.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?php
2+
3+
final class MetaMTAReceivedMailStatus
4+
extends MetaMTAConstants {
5+
6+
const STATUS_DUPLICATE = 'err:duplicate';
7+
const STATUS_FROM_PHABRICATOR = 'err:self';
8+
9+
}

‎src/applications/metamta/controller/PhabricatorMetaMTAReceiveController.php

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@ public function processRequest() {
1010

1111
if ($request->isFormPost()) {
1212
$received = new PhabricatorMetaMTAReceivedMail();
13-
$header_content = array();
13+
$header_content = array(
14+
'Message-ID' => Filesystem::readRandomBytes(12),
15+
);
1416
$from = $request->getStr('sender');
1517
$to = $request->getStr('receiver');
1618
$uri = '/mail/received/';
@@ -42,11 +44,6 @@ public function processRequest() {
4244
'text' => $request->getStr('body'),
4345
));
4446

45-
// Make up some unique value, since this column isn't nullable.
46-
$received->setMessageIDHash(
47-
PhabricatorHash::digestForIndex(
48-
Filesystem::readRandomBytes(12)));
49-
5047
$received->save();
5148

5249
$received->processReceivedMail();

‎src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,6 @@ public function processRequest() {
3939
'text' => $request->getStr('text'),
4040
'html' => $request->getStr('from'),
4141
));
42-
$received->setMessageIDHash(
43-
PhabricatorHash::digestForIndex($received->getMessageID()));
4442

4543
$file_phids = array();
4644
foreach ($_FILES as $file_raw) {

‎src/applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php
2+
3+
final class PhabricatorMetaMTAReceivedMailProcessingException
4+
extends Exception {
5+
6+
private $statusCode;
7+
8+
public function getStatusCode() {
9+
return $this->statusCode;
10+
}
11+
12+
public function __construct($status_code /* ... */) {
13+
$args = func_get_args();
14+
$this->statusCode = $args[0];
15+
16+
$args = array_slice($args, 1);
17+
call_user_func_array(array('parent', '__construct'), $args);
18+
}
19+
20+
}

‎src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php

Lines changed: 84 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
55
protected $headers = array();
66
protected $bodies = array();
77
protected $attachments = array();
8+
protected $status = '';
89

910
protected $relatedPHID;
1011
protected $authorPHID;
1112
protected $message;
12-
protected $messageIDHash;
13+
protected $messageIDHash = '';
1314

1415
public function getConfiguration() {
1516
return array(
@@ -25,18 +26,31 @@ public function setHeaders(array $headers) {
2526
// Normalize headers to lowercase.
2627
$normalized = array();
2728
foreach ($headers as $name => $value) {
28-
$normalized[strtolower($name)] = $value;
29+
$name = $this->normalizeMailHeaderName($name);
30+
if ($name == 'message-id') {
31+
$this->setMessageIDHash(PhabricatorHash::digestForIndex($value));
32+
}
33+
$normalized[$name] = $value;
2934
}
3035
$this->headers = $normalized;
3136
return $this;
3237
}
3338

39+
public function getHeader($key, $default = null) {
40+
$key = $this->normalizeMailHeaderName($key);
41+
return idx($this->headers, $key, $default);
42+
}
43+
44+
private function normalizeMailHeaderName($name) {
45+
return strtolower($name);
46+
}
47+
3448
public function getMessageID() {
35-
return idx($this->headers, 'message-id');
49+
return $this->getHeader('Message-ID');
3650
}
3751

3852
public function getSubject() {
39-
return idx($this->headers, 'subject');
53+
return $this->getHeader('Subject');
4054
}
4155

4256
public function getCCAddresses() {
@@ -156,35 +170,15 @@ private function getPhabricatorToInformation() {
156170

157171
public function processReceivedMail() {
158172

159-
// If Phabricator sent the mail, always drop it immediately. This prevents
160-
// loops where, e.g., the public bug address is also a user email address
161-
// and creating a bug sends them an email, which loops.
162-
$is_phabricator_mail = idx(
163-
$this->headers,
164-
'x-phabricator-sent-this-message');
165-
if ($is_phabricator_mail) {
166-
$message = "Ignoring email with 'X-Phabricator-Sent-This-Message' ".
167-
"header to avoid loops.";
168-
return $this->setMessage($message)->save();
169-
}
170-
171-
$message_id_hash = $this->getMessageIDHash();
172-
if ($message_id_hash) {
173-
$messages = $this->loadAllWhere(
174-
'messageIDHash = %s',
175-
$message_id_hash);
176-
$messages_count = count($messages);
177-
if ($messages_count > 1) {
178-
$first_message = reset($messages);
179-
if ($first_message->getID() != $this->getID()) {
180-
$message = sprintf(
181-
'Ignoring email with message id hash "%s" that has been seen %d '.
182-
'times, including this message.',
183-
$message_id_hash,
184-
$messages_count);
185-
return $this->setMessage($message)->save();
186-
}
187-
}
173+
try {
174+
$this->dropMailFromPhabricator();
175+
$this->dropMailAlreadyReceived();
176+
} catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) {
177+
$this
178+
->setStatus($ex->getStatusCode())
179+
->setMessage($ex->getMessage())
180+
->save();
181+
return $this;
188182
}
189183

190184
list($to,
@@ -460,4 +454,61 @@ private function lookupSender() {
460454
return $user;
461455
}
462456

457+
/**
458+
* If Phabricator sent the mail, always drop it immediately. This prevents
459+
* loops where, e.g., the public bug address is also a user email address
460+
* and creating a bug sends them an email, which loops.
461+
*/
462+
private function dropMailFromPhabricator() {
463+
if (!$this->getHeader('x-phabricator-sent-this-message')) {
464+
return;
465+
}
466+
467+
throw new PhabricatorMetaMTAReceivedMailProcessingException(
468+
MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR,
469+
"Ignoring email with 'X-Phabricator-Sent-This-Message' header to avoid ".
470+
"loops.");
471+
}
472+
473+
/**
474+
* If this mail has the same message ID as some other mail, and isn't the
475+
* first mail we we received with that message ID, we drop it as a duplicate.
476+
*/
477+
private function dropMailAlreadyReceived() {
478+
$message_id_hash = $this->getMessageIDHash();
479+
if (!$message_id_hash) {
480+
// No message ID hash, so we can't detect duplicates. This should only
481+
// happen with very old messages.
482+
return;
483+
}
484+
485+
$messages = $this->loadAllWhere(
486+
'messageIDHash = %s ORDER BY id ASC LIMIT 2',
487+
$message_id_hash);
488+
$messages_count = count($messages);
489+
if ($messages_count <= 1) {
490+
// If we only have one copy of this message, we're good to process it.
491+
return;
492+
}
493+
494+
$first_message = reset($messages);
495+
if ($first_message->getID() == $this->getID()) {
496+
// If this is the first copy of the message, it is okay to process it.
497+
// We may not have been able to to process it immediately when we received
498+
// it, and could may have received several copies without processing any
499+
// yet.
500+
return;
501+
}
502+
503+
$message = sprintf(
504+
'Ignoring email with message id hash "%s" that has been seen %d '.
505+
'times, including this message.',
506+
$message_id_hash,
507+
$messages_count);
508+
509+
throw new PhabricatorMetaMTAReceivedMailProcessingException(
510+
MetaMTAReceivedMailStatus::STATUS_DUPLICATE,
511+
$message);
512+
}
513+
463514
}

‎src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase {
4+
5+
protected function getPhabricatorTestCaseConfiguration() {
6+
return array(
7+
self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
8+
);
9+
}
10+
11+
public function testDropSelfMail() {
12+
$mail = new PhabricatorMetaMTAReceivedMail();
13+
$mail->setHeaders(
14+
array(
15+
'X-Phabricator-Sent-This-Message' => 'yes',
16+
));
17+
$mail->save();
18+
19+
$mail->processReceivedMail();
20+
21+
$this->assertEqual(
22+
MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR,
23+
$mail->getStatus());
24+
}
25+
26+
27+
public function testDropDuplicateMail() {
28+
$mail_a = new PhabricatorMetaMTAReceivedMail();
29+
$mail_a->setHeaders(
30+
array(
31+
'Message-ID' => 'test@example.com',
32+
));
33+
$mail_a->save();
34+
35+
$mail_b = new PhabricatorMetaMTAReceivedMail();
36+
$mail_b->setHeaders(
37+
array(
38+
'Message-ID' => 'test@example.com',
39+
));
40+
$mail_b->save();
41+
42+
$mail_a->processReceivedMail();
43+
$mail_b->processReceivedMail();
44+
45+
$this->assertEqual(
46+
MetaMTAReceivedMailStatus::STATUS_DUPLICATE,
47+
$mail_b->getStatus());
48+
}
49+
50+
51+
52+
}

‎src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,6 +1294,10 @@ public function getPatches() {
12941294
'type' => 'php',
12951295
'name' => $this->getPatchPath('20130508.releephtransactionsmig.php'),
12961296
),
1297+
'20130513.receviedmailstatus.sql' => array(
1298+
'type' => 'sql',
1299+
'name' => $this->getPatchPath('20130513.receviedmailstatus.sql'),
1300+
),
12971301
);
12981302
}
12991303
}

0 commit comments

Comments
 (0)