Skip to content

Commit

Permalink
Bug: 14324 Be more graceful with rfc822/message attachment failures.
Browse files Browse the repository at this point in the history
If the message fails to send, check that it's not because of a broken
rfc822/message attachement - most likely a too long header line. This is
done by re-encoding the attachment as a base64 encoded
application/octet-stream part. Not ideal, but we don't want to alter the
original message in anyway, and this at least allows it to be forwarded.
  • Loading branch information
mrubinsk committed Jun 14, 2016
1 parent 39801c1 commit be11249
Showing 1 changed file with 53 additions and 0 deletions.
53 changes: 53 additions & 0 deletions framework/Mime/lib/Horde/Mime/Part.php
Expand Up @@ -159,6 +159,13 @@ class Horde_Mime_Part
*/
protected $_transferEncoding = self::DEFAULT_ENCODING;

/**
* Flag to detect if a message failed to send at least once.
*
* @var boolean
*/
protected $_failed = false;

/**
* Constructor.
*/
Expand Down Expand Up @@ -1557,6 +1564,19 @@ public function send($email, $headers, Horde_Mail_Transport $mailer,
'canonical' => $canonical,
'charset' => $this->getHeaderCharset()
)), $msg);
} catch (InvalidArgumentException $e) {

This comment has been minimized.

Copy link
@yunosh

yunosh Jun 14, 2016

Member

InvalidArgumentExceptions are program errors and thus should never have to be catched.

This comment has been minimized.

Copy link
@mrubinsk

mrubinsk Jun 14, 2016

Author Member

InvalidArgumentExceptions are program errors and thus should never have to be catched.

Then Horde_Smtp is throwing an incorrect exception, but changing it would be a BC break.

This comment has been minimized.

Copy link
@yunosh

yunosh Jun 14, 2016

Member

Well, technically Horde_Smtp is correct, because we pass it an object that's only valid for BINARY even though the server advertised it doesn't support that.

This comment has been minimized.

Copy link
@mrubinsk

mrubinsk Jun 14, 2016

Author Member

Well, technically Horde_Smtp is correct, because we pass it an object that's only valid for BINARY even though the server advertised it doesn't support that.

I get that, but the checks that determine that the current server isn't able to satisfy the requirements of the object aren't done until we are already inside this method.

Getting a little more philosophical and disregarding the cause of this issue which I agree is probably better fixed in a way like you described in the other comments, if we are expected to only pass in objects that we already know can be successfully sent using the current SMTP server's capabilities then shouldn't we be checking such things before calling sent()? IMO, it's better to be able to catch this error and possibly react to it, then to have to add additional code to perform these checks everywhere we use the smtp object? If not, this should be documented in the phpdoc - that the objects must already be checked for the need for any potentially missing SMTP capabilities.

I'm trying to clarify this since catching the exception is also relied on in ActiveSync for another issue.

// Try to rebuild the part in case it was due to
// an invalid line length in a rfc822/message attachment.
if ($this->_failed) {
throw $e;
}
$this->_failed = true;
$this->_sanityCheckRfc822Attachments();
try {
$this->send($email, $headers, $mailer, $opts);
} catch (Horde_Mail_Exception $e) {
throw new Horde_Mime_Exception($e);
}
} catch (Horde_Mail_Exception $e) {
throw new Horde_Mime_Exception($e);
}
Expand Down Expand Up @@ -2113,6 +2133,39 @@ protected static function _findBoundary($text, $pos, $boundary,
return $out;
}

/**
* Re-enocdes message/rfc822 parts in case there was e.g., some broken
* line length in the headers of the message in the part. Since we shouldn't
* alter the original message in any way, we simply reset cause the part to
* be encoded as base64 and sent as a application/octet part.
*/
protected function _sanityCheckRfc822Attachments()
{
if ($this->getType() == 'message/rfc822') {
$this->_reEncodeMessageAttachment($this);
return;
}
foreach ($this->getParts() as $part) {
if ($part->getType() == 'message/rfc822') {
$this->_reEncodeMessageAttachment($part);
}
}
return;
}

/**
* Rebuilds $part and forces it to be a base64 encoded
* application/octet-stream part.
*
* @param Horde_Mime_Part $part The MIME part.
*/
protected function _reEncodeMessageAttachment(Horde_Mime_Part $part)
{
$new_part = Horde_Mime_Part::parseMessage($part->getContents());
$part->setContents($new_part->getContents(array('stream' => true)), array('encoding' => self::ENCODE_BINARY));
$part->setTransferEncoding('base64', array('send' => true));
}

/* ArrayAccess methods. */

/**
Expand Down

5 comments on commit be11249

@yunosh
Copy link
Member

@yunosh yunosh commented on be11249 Jun 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be sufficient to enforce a binary-safe encoding, i.e. to enforce base64 transfer encoding? The message part should still be a message/rfc822 mime part. A broken one, granted, but that's what the receiving client could deal with.

@mrubinsk
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be sufficient to enforce a binary-safe encoding, i.e. to enforce base64 transfer encoding? The message part should still be a message/rfc822 mime part. A broken one, granted, but that's what the receiving client could deal with.

I had tried that. I.e., just setting $part->setTransferEncoding('base64', array('send' => true)); but this didn't work. I will investigate further as to why this is the case.

@yunosh
Copy link
Member

@yunosh yunosh commented on be11249 Jun 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking further through this code, the problem seems to be that we don't apply the _getTransferEncoding() algos if the current mime part is of primary type 'message' (line 1009). We just assume that those parts are correctly encoded already.
Thus we either need to apply this logic on 'message' parts too, though there probably is a good reason that we don't. Or we must not assume that messages that we receive from the IMAP server are valid MIME messages, which is probably a good idea.
TL;DR We should check message we receive from the IMAP and not just attach them as-is but check first if we need to apply a different encoding.

@mrubinsk
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. Don't know how I missed that line. I'll check into that.

@mrubinsk
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be sufficient to enforce a binary-safe encoding, i.e. to enforce base64 transfer encoding? The message part should still be a message/rfc822 mime part. A broken one, granted, but that's what the receiving client could deal with.

The problem here is that message parts of type message/rfc822 can only have '7bit', '8bit', or 'binary' as the value for Content-Transfer-Encoding. So, even if we change where we catch the fact that the message/rfc822 part is not valid for SMTP servers without BINARY, we still have to send it as a Application/Octet-Stream, since sending it as Base64 encoded message/rfc822 part is also invalid.

Such parts will not display correctly in other clients either. For example, if Content-Type is message/rfc822 and Content-Transfer-Encoding is Base64, the raw base64 string will show in IMP as the message body, and in Thunderbird nothing is shown - no body text, no attachment available, nothing. If we send it as Base64 encoded Octet-Stream, it shows as an attachment in both IMP and Thunderbird, but more importantly, clicking on the attachment actually displays the message properly. IMO, it's better to send it as a part that will display as an attachment, rather than a part that will display garbage (or nothing at all) to the user.

Now, the question becomes if we should check for this ahead of time (and never rely on the SMTP server having BINARY available), or deal with it if we discover the SMTP server couldn't send it (as the fix already in place does).

Please sign in to comment.