When forwarding an encrypted message as attachment, the attached message should get decrypted before it is attached. This is necessary to allow new recipients to be able to read the attached objects. See also bug 141730, which already cared for the reply to and forward inline scenarios. Jean-Francois said, this bug will be more difficult to implement (than 141730), because when forwarding as attachment, the messages is not piped through mime.

This patch was created as teamwork by Jean-Francois and me, with 90% of the ideas coming from JF.

Comment on attachment 105568 [details] [diff] [review] Patch v1 r=kaie on the portions that were created by JF. In addition I have tested the patch and it seems to work for me. JF, could you review the portions that came from me, so we have a fully reviewed patch?

Comment on attachment 105568 [details] [diff] [review] Patch v1 m_mime_parser doesn't have to be a member variable but that's fine with me. However, it's important to hold a grop on m_converter_channel. Maybe in the future MIME should own the channel but that's another story. I have tested the patch to be sure we are not leaking, everything seems fine. R=ducarroz

Comment on attachment 105568 [details] [diff] [review] Patch v1 1) please don't add the static cid. (can fix it in nsMsgCreate.cpp and sMsgQuote.cpp, too? If we can get rid of all at once, it will prevent copy and paste.) instead, use the contract id (NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID) and use do_CreateInstance(). 2) + if (!msd->options->decrypt_p) msd->options->write_html_p = PR_TRUE; is write_html_p initialized before this? I assume we don't want: msd->options->write_html_p = !msd->options->decrypt_p; because that would override write_htmp_p in certain cases to be false, when it should be true 3) + rv = nsComponentManager::CreateInstance(kStreamConverterCID, + NULL, NS_GET_IID(nsIStreamConverter), + (void **) getter_AddRefs(m_mime_parser)); + + if (NS_FAILED(rv) || !m_mime_parser) + { + if (NS_SUCCEEDED(rv)) + rv = NS_ERROR_UNEXPECTED; + goto done; + } switch to do_CreateInstance(). it's not going to succeeded and yet not give you the instance. so just do: m_mime_parser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv); if (NS_FAILED(rv)) goto done; or: 4) + nsCOMPtr<nsIStreamListener> convertedListener = do_QueryInterface(m_mime_parser); + if (!convertedListener) + { + if (NS_SUCCEEDED(rv)) + rv = NS_ERROR_UNEXPECTED; + goto done; + } do: + nsCOMPtr<nsIStreamListener> convertedListener = do_QueryInterface(m_mime_parser, &rv); + if (NS_FAILED(rv)) + goto done; 6) + rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL, nsnull, + NS_LITERAL_CSTRING(""), NS_LITERAL_CSTRING(""), -1); + if (NS_FAILED(m_mime_parser->AsyncConvertData( + NS_LITERAL_STRING("message/rfc822").get(), + NS_LITERAL_STRING("message/rfc822").get(), + strListener, m_converter_channel))) + { + if (NS_SUCCEEDED(rv)) + rv = NS_ERROR_UNEXPECTED; + goto done; + } why not: + rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL, nsnull, + NS_LITERAL_CSTRING(""), NS_LITERAL_CSTRING(""), -1); + if (NS_FAILED(rv)) + goto done; + + rv = m_mime_parser->AsyncConvertData( + NS_LITERAL_STRING("message/rfc822").get(), + NS_LITERAL_STRING("message/rfc822").get(), + strListener, m_converter_channel); + if (NS_FAILED(rv)) + goto done;

> 1) > > please don't add the static cid. (can fix it in nsMsgCreate.cpp and > sMsgQuote.cpp, too? If we can get rid of all at once, it will prevent copy and > paste.) > > instead, use the contract id (NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID) and > use do_CreateInstance(). Fixed, including some calls in MsgCreate and MsgQuote, but only where there was an contract ID constant already available. > 2) > > + if (!msd->options->decrypt_p) > msd->options->write_html_p = PR_TRUE; > > is write_html_p initialized before this? > > I assume we don't want: > > msd->options->write_html_p = !msd->options->decrypt_p; > > because that would override write_htmp_p in certain cases to be false, when it > should be true I don't know, I must leave this comment to Jean-Francois. What do you think? > 3) changed > 4) changed there was no 5... > 6) changed

Jean-Francois, could you please check whether we need to address Seth's comment 2?

Oops, in the previous patch, I had not correctly compiled in compose, this new patch corrects a wrong bracket in the call to AsyncConvertData.

the best way to address Seth comment #2 is to reorder the initialization of write_html_p: Index: mimemoz2.cpp =================================================================== RCS file: /cvsroot/mozilla/mailnews/mime/src/mimemoz2.cpp,v retrieving revision 1.184 diff -u -2 -w -r1.184 mimemoz2.cpp --- mimemoz2.cpp 4 Oct 2002 22:40:53 -0000 1.184 +++ mimemoz2.cpp 12 Nov 2002 18:18:06 -0000 @@ -1518,4 +1518,5 @@ // MIME_HeaderType = MimeHeadersAll; + msd->options->write_html_p = PR_TRUE; switch (format_out) { @@ -1544,4 +1545,9 @@ case nsMimeOutput::nsMimeMessageFilterSniffer: // generating an output that can be scan by a message filter break; + + case nsMimeOutput::nsMimeMessageDecrypt: + msd->options->decrypt_p = PR_TRUE; + msd->options->write_html_p = PR_FALSE; + break; } @@ -1581,5 +1587,4 @@ msd->options->url = msd->url_name; - msd->options->write_html_p = PR_TRUE; msd->options->output_init_fn = mime_output_init_fn;

This patch replaces the changes to mimemoz2.cpp with the new code from JF.

Comment on attachment 105988 [details] [diff] [review] Patch v4 Carrying forward the existing reviews for the trivial changes requested by Seth. r=kaie on the new logic for mimemoz2.cpp, it still works for me

Comment on attachment 105988 [details] [diff] [review] Patch v4 sr=sspitzer thanks for the fix, and the code cleanup! two nits: 1) + // initialize a new stream converter, that uses the strListener as its input + // obtain the input stream listener from the new converter, + // and pass the converter's input stream listener to DisplayMessage + + m_mime_parser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv); + if (NS_FAILED(rv) || !m_mime_parser) + { + if (NS_SUCCEEDED(rv)) + rv = NS_ERROR_UNEXPECTED; + goto done; + } just do: + m_mime_parser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv); + if (NS_FAILED(rv)) + goto done; 2) + mimeParser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv); if (NS_FAILED(rv) || !mimeParser) since you are fixing this code, can you do: + mimeParser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv); + if (NS_FAILED(rv)) { Release();

as requested

Comment on attachment 105994 [details] [diff] [review] Patch v5 carrying forward reviews

Fixed on trunk.

20021204 Trnk builds - OSX, Win2k, and Linux7.3 - verified.

this fixed caused a regression. see http://bugzilla.mozilla.org/show_bug.cgi?id=188344

this fixed caused a regression. see http://bugzilla.mozilla.org/show_bug.cgi?id=188344

this caused another regression, see #189988

And we found another regression that was caused by this bug. A message, containing a base 64 encoded object as it's single content, e.g. a GIF image (not a multipart message), will arrive damaged when forwarded as attachment. Actually, the image will get forwarded in its decoded binary form.

The regression that I mentioned in the previous comment has been filed as bug 194636.