Created attachment 702 [details]
Example mail -- source

Created attachment 703 [details]
Example mail -- screenshot from Mail.app

Note: the screenshot has some binary garbage at the beginning of the file,
but if you remove the first 734 bytes up to "\x89PNG", then the PNG image is
fine.

assigning to me, changing summary

*** Bug 1618 has been marked as a duplicate of this bug. ***

Created attachment 736 [details]
Spam screenshot

Created attachment 737 [details]
Actual spam text

The above attachments I just uploaded (736 and 737) were on Mozilla v1.1 under
RedHat Linux v7.1.

Created attachment 759 [details]
proposed fix

Mark and Rob: please test the proposed fix as soon as possible.  I would
like to get your feedback in time to make any necessary revisions and get
this change in 2.51 (which freezes on Tuesday and is released on Thursday).

After you apply the patch, all you have to do is set report_safe to 2 and
then the attachment is text/plain instead of message/rfc822.  I really need
to know that this will render the attachment safe on Apple Mail and Mozilla
(by safe, I mean, won't load HTML in the spam automatically).

Thanks!

Created attachment 760 [details]
backport of patch for 2.51

I improved the documentation text a bit in the backport for 2.51 version.
It applies to HEAD as well, so please consider that the proposal.

The patch works with Apple Mail.app.  It might be nice to also have the option to have a 
text version displayed inline (the old way) rather than as an attachment, so content could 
be examined quickly to see if the mail is legit.

However, this is a vast improvement over only have the message/rfc822 option, and it 
solves the issue.

Thanks for your attention!

Subject: Re:  need safer way to render attachments for broken clients

mark@antsclimbtree.com writes:

> The patch works with Apple Mail.app.  It might be nice to also have
> the option to have a text version displayed inline (the old way)
> rather than as an attachment, so content could be examined quickly to
> see if the mail is legit.

Well, that's the idea behind the content preview.

I'm trying to figure out if there's some change or addition we could
make that would please the people who liked the old destructive behavior
without having the same negative qualities (well, and also just trying
to figure out what they want, there was a very long thread about it on
SAtalk).  Regardless, I think that level of change should just wait for
the 2.60 release.

> However, this is a vast improvement over only have the message/rfc822
> option, and it solves the issue.
>
> Thanks for your attention!

And thanks for testing it quickly.  :-)

OKAY: looks good to me 
 
A general remark: I don't like the check for encapsulated_content_description 
to find out if this is really a SA encapsulated message; if somebody changes 
the encapsulated_content_description, all old mails won't be recognized 
anymore (or am I wrong here?). I think a better solution would be to add to 
the header of the attachment either an 
  X-Spam-Encapsulated: yes 
header, or, which I prefer, a parameter to its ct like 
  Content-Type: message/rfc822; x-spam-encapsulated=yes 
 

Subject: Re:  need safer way to render attachments for broken clients

spamassassin-contrib@msquadrat.de writes:

> A general remark: I don't like the check for encapsulated_content_description
> to find out if this is really a SA encapsulated message; if somebody changes
> the encapsulated_content_description, all old mails won't be recognized
> anymore (or am I wrong here?). I think a better solution would be to add to
> the header of the attachment either an
>   X-Spam-Encapsulated: yes 
> header, or, which I prefer, a parameter to its ct like 
>   Content-Type: message/rfc822; x-spam-encapsulated=yes 

I agree 100%, that was annoying me.  I like your idea (same preference
for the latter solution).  Let me check the MIME RFCs for how
self-defined parameters are handled.  In the check, we can allow either
the encapsulated_content_description or the parameter.

Daniel

I think for backward compatibility SA should fall back to the description. 
 
I checked the RFC 2045 and in 5.1 it doesn't explicitly allow x-attributes but 
doesn't disallow them, too. It says defining attributes is up to the MIME 
type, so it seems like every MIME type has to allow x-attributes for its own. 
As we use only message/rfc822 and text/plain, we'd have to check for those 
MIME types if they do. I think I read somewhere (can't find that sentence 
again) that unknown parameters should be ignored, so if the MIME types don't 
allow x-attributes explicitly, we could go and assume that They won't ever 
assign x-names. 

Subject: Re:  need safer way to render attachments for broken clients

spamassassin-contrib@msquadrat.de writes:

> I think for backward compatibility SA should fall back to the description.
>
> I checked the RFC 2045 and in 5.1 it doesn't explicitly allow x-attributes
> but doesn't disallow them, too. It says defining attributes is up to the
> MIME type, so it seems like every MIME type has to allow x-attributes for
> its own.  As we use only message/rfc822 and text/plain, we'd have to check
> for those MIME types if they do. I think I read somewhere (can't find that
> sentence again) that unknown parameters should be ignored, so if the MIME
> types don't allow x-attributes explicitly, we could go and assume that
> They won't ever assign x-names.

I like the idea of using a Content-Type parameter somewhat better than
adding a new MIME header.  I checked my ham corpus and found the following
Content-Type parameters (for all MIME content types).

  count parameter      applicability
  ----- -------------  --------------------
   7058 charset        nope
    512 format         nope, it's always "flowed"
    323 boundary       nope
    113 name	       maybe
     94 protocol       nope
     93 micalg	       nope
     37 report-type    nope (DSA only)
      5 x-mac-type     ah, some precedent for an x- parameter
      5 x-mac-creator  ditto

I'd either make out our own x-foo parameter or use "name".  How about this?

  Content-Type: message/rfc822; x-spam-type=original

(I had "encapsulated" as the value instead of "original", but that seems
redundant.  "original" also matches the default description.)

That leaves us some options for other x-spam-types.  I'm afraid if we use
"name", then users will be more tempted to change it.

Daniel

Sounds good to me. I think we could make the value of x-spam-type a comma- 
separated list to be forward-compatible with whatever we might come into our 
minds in future. 

Created attachment 767 [details]
revised patch for HEAD, please review (and test for your MUA if you have time)

Created attachment 768 [details]
revised patch for 2.50 branch, please review (and test for your MUA if you have time)

The new patch works on my end.  I'm not 100% positive how to test the change, or what the latest change does exactly, but it didn't break anything.

> The new patch works on my end.  I'm not 100% positive how to test the change,
> or what the latest change does exactly, but it didn't break anything.

Thanks.  The change adds a parameter to the Content-Type of the message
attachment.  It shouldn't cause problems for any MUAs, but it's good to
hear that it works on at least one quirky client.  ;-)

So, before you had:

  Content-Type: text/plain

now you'll have:

  Content-Type: text/plain; x-spam-type=original

(Same change if using report_safe of 1 with message/rfc822 has the type.)

Sorry I haven't had time to test the patch out yet. But I assume that if it
worked for the Mac MUA, it should work for Linux. I'll let you know later tonight.

Dan, any chance you could just check in the HEAD patch?   That'll force
all us dogfooders to test it, and it *is* bleeding-edge CVS anyway ;)

Subject: Re:  need safer way to render attachments for broken clients

jm@jmason.org writes:

> Dan, any chance you could just check in the HEAD patch?  That'll force
> all us dogfooders to test it, and it *is* bleeding-edge CVS anyway ;)

Done!  Checked into HEAD.

I'm pretty happy with the current version.  I also experimented with a
"report_original_type" variable (set either to "message" or "text"), but
this is more intuitive, cleaner, one option less, and is even shorter.  :-)

Malte's idea to add a parameter also makes extraction much cleaner than
relying on the description (which is likely to be localized).

Can someone review and OKAY the 2.50 version?

Will investigate 2.50 patches tonight.

OKAY: looks good to me

OKAY: looks good and works (at least with KMail) 

applied to 2.50 branch, thanks everyone