Failed Patch caused lost messages.

[rmiddle]

The following patch failed to apply correctly into my tree and started getting the following error. [WARN]: [Parser] Invalid 'From' address: Array. But my bigger concern is that the mail was lost. I though if the Parser failed the messages were moved in failed? We lost about 10 hours worth of tickets before I figured out was going on. The question I have is why wasn't the messages moved to fail. Note the issue with the patch is for so reason the new lines were added but the removed lines weren't removed. Should a failure in the pre-processor send the message to fail?

Thanks
Robert

http://github.com/wgm/cerb4/commit/5...3d5a7c58834b71

[jstanden]

It *should* leave failed messages in /storage/mail/fail; but how did your patch fail? Was it a bad merge with Git? The code can't always handle a bad merge gracefully -- especially with a subtle 'warning' level error that results from it.

You should not count on /storage/mail/fail to be your only defense, and then be surprised if something ate mail; especially if you run development versions or have local changes. If your business mail is important (and it should be) then send a copy of all your incoming mail to an 'audit' mailbox as well as the POP3 dropbox you have Cerb4 pull from. You can purge it every couple days from the mail server, but if you suspect a problem (like this) you can 'grep' the mail headers and move the messages into /storage/mail/new for delivery (by date or anything).

We do the same kind of thing on our live helpdesk. We have an external copy of every message we receive. We spot check it occasionally against the messages table. If we set up a filter that ate something, we can always recover it.

It should only take a couple minutes to set up a mail account that's both a mailbox and a redirect. You can even redirect a copy to Gmail or something. It's something the code has to be conscious of, but you need to be conscious of as an admin as well.

[rmiddle]

Originally Posted by jstanden

It *should* leave failed messages in /storage/mail/fail; but how did your patch fail? Was it a bad merge with Git? The code can't always handle a bad merge gracefully -- especially with a subtle 'warning' level error that results from it.

You should not count on /storage/mail/fail to be your only defense, and then be surprised if something ate mail; especially if you run development versions or have local changes. If your business mail is important (and it should be) then send a copy of all your incoming mail to an 'audit' mailbox as well as the POP3 dropbox you have Cerb4 pull from. You can purge it every couple days from the mail server, but if you suspect a problem (like this) you can 'grep' the mail headers and move the messages into /storage/mail/new for delivery (by date or anything).

We do the same kind of thing on our live helpdesk. We have an external copy of every message we receive. We spot check it occasionally against the messages table. If we set up a filter that ate something, we can always recover it.

It should only take a couple minutes to set up a mail account that's both a mailbox and a redirect. You can even redirect a copy to Gmail or something. It's something the code has to be conscious of, but you need to be conscious of as an admin as well.

I do have a copy of the emails in question. It just took a while to narrow down the problem. Yea it was a Git merge that failed. Although I pull master down pretty frequently to make sure I don't get any ugly surprises with my custom reports. But I only use the versions you push to SVN at least in theory on the live site. But looking at the failed patch it looks like you changed the format of from. This caused the function to return NULL. That should have cause the message to have been moved to fail or stop processing shouldn't it?

Link to the commit were I commented out the sections that was left in my code. This solved the processing problem.

http://github.com/rmiddle/cerb4/comm...e697c537ef58f2

Thanks
Robert

[rmiddle]

To make it clear. The problem was mine for the Parser failing. My concern is that since from format changed I was pushing an error and returning Null. That should have either stopped all processing or moved the mail to fail shouldn't it? I have't looked at the calling function to know how it handles Null. But the current code still returns Null on error? It also looks like it returns NULL on bounce, redirect, or blackhole. Does this mean that errors in this functions just throws the mail away?

Thanks
Robert

[jstanden]

I'd have to test and reproduce it. It's possible.

Though if "errors of this kind" mean merges that mix two difference releases into a single codebase, that shouldn't happen in production.

[rmiddle]

Originally Posted by jstanden

I'd have to test and reproduce it. It's possible.

Though if "errors of this kind" mean merges that mix two difference releases into a single codebase, that shouldn't happen in production.

Correct. The failed patch was causing an error to be returned that shouldn't be. But the error is returning Null the same as mail that has been properly processed. I am looking at the function as it is sitting on Git right now. Inside the function

static public function parseMessage(CerberusParserMessage $message, $options=array()) {

You have.

switch($action_key) {
case 'blackhole':
return NULL;
break;

and

if(null == ($fromAddressInst = CerberusParser::getAddressFromHeaders($headers))) {
$logger->err("[Parser] 'From' address could not be created.");
return NULL;
}

Thanks
Robert

[rmiddle]

Did a little more digging and found this is the code that calls that function. It doesn't do anything with the return at all. It just removes the file. So if some place else in the code it doesn't copy the file out then this will wipe it out. I think some error checking needs to be inserted.

PHP Code:

function _parseFile($full_filename) {
  $logger = DevblocksPlatform::getConsoleLog();

  $fileparts = pathinfo($full_filename);
  $logger->info("[Parser] Reading ".$fileparts['basename']."...");

  $time = microtime(true);

  $mime = mailparse_msg_parse_file($full_filename);
  $message = CerberusParser::parseMime($mime, $full_filename);

  $time = microtime(true) - $time;
  $logger->info("[Parser] Decoded! (".sprintf("%d",($time*1000))." ms)");

  //  echo "<b>Plaintext:</b> ", $message->body,"<BR>";
  //  echo "<BR>";
  //  echo "<b>HTML:</b> ", htmlspecialchars($message->htmlbody), "<BR>";
  //  echo "<BR>";
  //  echo "<b>Files:</b> "; print_r($message->files); echo "<BR>";
  //  echo "<HR>";

  $time = microtime(true);
  $ticket_id = CerberusParser::parseMessage($message);
  $time = microtime(true) - $time;

  $logger->info("[Parser] Parsed! (".sprintf("%d",($time*1000))." ms) " .
    (!empty($ticket_id) ? ("(Ticket ID: ".$ticket_id.")") : ("(Local Delivery Rejected.)")));

  //  This wipes the email out.  There is no error checking going on
  @unlink($full_filename);
  mailparse_msg_free($mime);

  //  flush();
} 


I see several ways to fix this function.

Thanks
Robert

[rmiddle]

Ok after have a good nights sleep I looked over the code and I think I can fix this with only a handful of changes. First up a few changes to ParseMessage

PHP Code:

    static public function parseMessage(CerberusParserMessage $message, $options=array()) {
...
  switch($action_key) {
    case 'blackhole':
      //return NULL;
      return -1;
      break;

    case 'redirect':
      @$to = $action['to'];
      CerberusMail::reflect($message, $to);
      //return NULL;
      return -1;
      break;

    case 'bounce':
      @$msg = $action['message'];
      @$subject = 'Delivery failed: ' . self::fixQuotePrintableString($message->headers['subject']);

      // [TODO] Follow the RFC spec on a true bounce
      if(null != ($fromAddressInst = CerberusParser::getAddressFromHeaders($message->headers))) {
        CerberusMail::quickSend($fromAddressInst->email,$subject,$msg);
      }
      //return NULL;
      return -1;
      break;
... 


Now for _parseFile

PHP Code:

function _parseFile($full_filename) {
...
  //  If NULL move it to failure.
  if ($ticket_id == NULL) {
    @unlink($full_filename);
    mailparse_msg_free($mime);
  } else {
    // Move to Fail Folder
  }
...
} 


There are two other places that might need a minor change in

plugins/usermeet.core/api/sc/uri/contact.php
plugins/cerberusweb.webapi/api/App.php