[Rt-commit] rt branch, 4.2/html-content-safety, created. rt-4.2.1-43-g254e8da

[Alex Vandiver]

The branch, 4.2/html-content-safety has been created
        at  254e8da98a2025ab3de31e751091bd048e3a0987 (commit)

- Log -----------------------------------------------------------------
commit 254e8da98a2025ab3de31e751091bd048e3a0987
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Mon Dec 2 12:44:32 2013 -0500

    Don't die if HTML → text conversion throws an error
    
    HTML::FormatText::WithLinks::AndTables may contain errors which make it
    incapable of rendering the HTML to text.  In the context of templates,
    this led to the outgoing mail being dropped on the floor if the
    conversion failed.  It also showed as errors in the REST interface and
    RSS feed, in attempting to downsample an HTML-only message to plain text
    to display therein.
    
    Expliticly wrap the conversion with an eval to trap fatal errors in the
    HTML::FormatText::WithLinks::AndTables module; in such cases, the method
    returns undef.  In the context of template generation, the presence of
    such a return value is to omit the text/plain part entirely, as email
    clients may be able to generate it even if we are unable to.
    
    It is expected that the new tests may begin to fail if
    HTML::FormatText::WithLinks::AndTables resolves its bug surrounding
    nested tables.  Unfortunately, marking them TODO is difficult because
    they intermix passing and failing tests in mail_ok.

diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm
index 444f842..57c4ed5 100644
--- a/lib/RT/Interface/Email.pm
+++ b/lib/RT/Interface/Email.pm
@@ -1765,8 +1765,9 @@ sub _RecordSendEmailFailure {
 
 =head2 ConvertHTMLToText HTML
 
-Takes HTML and converts it to plain text.  Appropriate for generating a plain
-text part from an HTML part of an email.
+Takes HTML and converts it to plain text.  Appropriate for generating a
+plain text part from an HTML part of an email.  Returns undef if
+conversion fails.
 
 =cut
 
@@ -1774,20 +1775,27 @@ sub ConvertHTMLToText {
     my $html = shift;
 
     require HTML::FormatText::WithLinks::AndTables;
-    return HTML::FormatText::WithLinks::AndTables->convert(
-        $html => {
-            leftmargin      => 0,
-            rightmargin     => 78,
-            no_rowspacing   => 1,
-            before_link     => '',
-            after_link      => ' (%l)',
-            footnote        => '',
-            skip_linked_urls => 1,
-            with_emphasis   => 0,
-        }
-    );
+    my $text;
+    eval {
+        $text = HTML::FormatText::WithLinks::AndTables->convert(
+            $html => {
+                leftmargin      => 0,
+                rightmargin     => 78,
+                no_rowspacing   => 1,
+                before_link     => '',
+                after_link      => ' (%l)',
+                footnote        => '',
+                skip_linked_urls => 1,
+                with_emphasis   => 0,
+            }
+        );
+        $text //= '';
+    };
+    $RT::Logger->error("Failed to downgrade HTML to plain text: $@") if $@;
+    return $text;
 }
 
+
 RT::Base->_ImportOverlays();
 
 1;
diff --git a/lib/RT/Template.pm b/lib/RT/Template.pm
index 43c08ba..ad57c3d 100644
--- a/lib/RT/Template.pm
+++ b/lib/RT/Template.pm
@@ -660,14 +660,17 @@ sub _DowngradeFromHTML {
 
     $orig_entity->head->mime_attr( "Content-Type" => 'text/html' );
     $orig_entity->head->mime_attr( "Content-Type.charset" => 'utf-8' );
-    $orig_entity->make_multipart('alternative', Force => 1);
 
     require Encode;
-    $new_entity->bodyhandle(MIME::Body::InCore->new(
-        # need to decode_utf8, see the doc of MIMEObj method
-        \(RT::Interface::Email::ConvertHTMLToText(Encode::decode_utf8($new_entity->bodyhandle->as_string)))
-    ));
+    my $body = $new_entity->bodyhandle->as_string;
+    # need to decode_utf8, see the doc of MIMEObj method
+    $body = Encode::decode_utf8( $body );
+    my $html = RT::Interface::Email::ConvertHTMLToText( $body );
+    return unless defined $html;
+
+    $new_entity->bodyhandle(MIME::Body::InCore->new( \$html ));
 
+    $orig_entity->make_multipart('alternative', Force => 1);
     $orig_entity->add_part($new_entity, 0); # plain comes before html
     $self->{MIMEObj} = $orig_entity;
 
diff --git a/t/mail/html-outgoing.t b/t/mail/html-outgoing.t
index d20dd63..1b460c2 100644
--- a/t/mail/html-outgoing.t
+++ b/t/mail/html-outgoing.t
@@ -4,10 +4,11 @@ use RT::Test tests => undef;
 BEGIN {
     plan skip_all => 'Email::Abstract and Test::Email required.'
         unless eval { require Email::Abstract; require Test::Email; 1 };
-    plan tests => 18;
+    plan tests => 22;
 }
 
 use RT::Test::Email;
+use Test::Warn;
 
 my $root = RT::User->new(RT->SystemUser);
 $root->Load('root');
@@ -86,6 +87,29 @@ mail_ok {
 };
 
 
+diag "Failing HTML -> Text conversion";
+warnings_like {
+    my $body = '<table><tr><td><table><tr><td>Foo</td></tr></table></td></tr></table>';
+    mail_ok {
+        ($ok, $tmsg) = $t->Correspond(
+            MIMEObj => HTML::Mason::Commands::MakeMIMEEntity(
+                Body => $body,
+                Type => 'text/html',
+            ),
+        );
+    } { from    => qr/RT System/,
+        bcc     => 'root at localhost',
+        subject => qr/\Q[example.com #1] The internet is broken\E/,
+        body    => qr{Ticket URL: <a href="(http://localhost:\d+/Ticket/Display\.html\?id=1)">\1</a>.+?$body}s,
+        'Content-Type' => qr{text/html},  # TODO
+    },{ from    => qr/RT System/,
+        to      => 'enduser at example.com',
+        subject => qr/\Q[example.com #1] The internet is broken\E/,
+        body    => qr{<table><tr><td><table><tr><td>Foo</td></tr></table></td></tr></table>},
+        'Content-Type' => qr{text/html},  # TODO
+    };
+} [(qr/uninitialized value/, qr/Failed to downgrade HTML/)x3];
+
 diag "Admin Comment in HTML";
 mail_ok {
     ($ok, $tmsg) = $t->Comment(

-----------------------------------------------------------------------