I think this slipped when bug 465113 was fixed: If an encrypted message (S/MIME or PGP/MIME doesn't matter) contains an entire embedded message such as when doing "Forward as attachment", or if following RFC 3156, section 6.1, then you'll get the following error: Error: parentPart is undefined Source File: file:.../components/jsmimeemitter.js Line: 311 The following change to jsmimeemitter.js fixes this: - let oldPart = parentPart.parts[childIndex]; - parentPart.parts[childIndex] = aPart; - // copy over information from the original part - aPart.parts = oldPart.parts; - aPart.headers = oldPart.headers; + if (parentPart !== undefined) { + let oldPart = parentPart.parts[childIndex]; + parentPart.parts[childIndex] = aPart; + // copy over information from the original part + aPart.parts = oldPart.parts; + aPart.headers = oldPart.headers; + }

We definitely don't handle this yet. (gloda/test/unit/test_mime_emitter.js does not generate multipart/encrypted streams) The parentPart change was made so errors like this would stick out. Unfortunately, it's a bit of a painful failure mode. I expect there may be a libmime change involved to properly fix this...

Andrew, do you have any advice as to how to fix this, and/or how to setup a test environment that can allow me to do xpcshells with enigmail enabled? I'd like the JSMimeEmitter to output a decrypted representation of a multipart/encrypted mime/pgp message... so that I could land bug 564423 without afterthoughts.

It sounds like from Patrick's initial comment that you should be able to reproduce the problem by having our JS mime emitter test generate an S/MIME encrypted hierarchy. (We only generate signed hierarchies right now.) Unless I'm misunderstanding something and enigmail's binary components need to generate the events the JS mime emitter wants?

(In reply to comment #3) > It sounds like from Patrick's initial comment that you should be able to > reproduce the problem by having our JS mime emitter test generate an S/MIME > encrypted hierarchy. (We only generate signed hierarchies right now.) I assume that if the problem is fixed for S/MIME encrypted messages then Enigmail should be covered as well. This was always the case in the past.

Fundamentally, the assumption we should've known about the parent having us as a subpart beforehand is wrong. UUencoded attachments, for instance, are not wrapped in a MIME part at all, which is why we get that call to _replacePart out of the blue. I'll try to see if I can fix this.

So, turns out I wanted to take a shot at making the JS Mime Emitter compatible with UUencoded attachments (I want to make users happy, I guess that's my problem). Turns out it triggered the exact same problem described on the bug, which is good because: - it motivated me more to fix it and, - it gave me a simple way to write a test, since I had some ready-made uuencoded attachments lurking around in test files from last summer. UUencoded attachments are just text inside an email body, they're not enclosed in any MIME part, so we're completely confused when we encounter them. What the patch does is make the JS Mime emitter brave: we now try to find a place to sit that part in. That allows Thunderbird Conversations to now display properly UUencoded attachments. Isn't it fantastic?

And because this is a work in progress, there's a lot of debug info in there. But the upside is, running this patch in a unix machine will give you lots of colorful cryptic stuff in the terminal. Patrick, I'd love if you could give this a try.

Comment on attachment 550181 [details] [diff] [review] Dumb patch More problems: this patch now makes the JS Mime Emitter see through multipart/encrypted parts which it should always discard unless explicitly instructed to do so. I'm going to kill two birds with one stone and also fix this.

Patrick, this patch does something more sophisticated, and makes sure no part is lost when building the MimeMessage representation of the message after it has been decrypted. Here, I'm only building what libmime notifies to the jsmimeemitter, so if there's some part that you would like to see, and that isn't there, it should be fixed on the libmime side rather than the jsmimeemitter.js one. What you can do is send me a message with a signature, and tell me which part is missing before decryption, and after decryption. That would help me as RFC numbers don't help me much understanding which type of message is problematic for you. Thanks! :-)

This patch is not complete, as it lacks proper tests to make sure Gloda is not indexing encrypted content. However, it does the fallback hierarchy through the uuencoded-attachment. Andrew, this patch is to apply on top of the patch from bug 564423 (and before the patch for bug 655536).

Comment on attachment 550550 [details] [diff] [review] Cleaned up patch This is looking good. I must concede I am slightly unsettled by the "no encryption" case being handled by a post-pass, but I think my concern is baseless since: 1) XPConnect is already going to cause the strings to be held in unlocked memory. 2) A flaw in our exclusion logic would result in the exact same result if we did it inside the emitter just as the same as outside the emitter. 3) Rogue code can just request a streaming with the encrypted data kept. Having said that, please make sure the encrypted tests cover and explicitly call out the various permutations, both of S/MIME and Enigmail's PGP usage, even though we will not test the enigmail case in-tree. Some additional rich comments, also inlined here: http://reviews.visophyte.org/r/550550/ on file: mailnews/db/gloda/components/jsmimeemitter.js line 352 > let index = 0; > while (index in parentPart.parts) > index++; It's an array, there's no need for this. on file: mailnews/db/gloda/components/jsmimeemitter.js line 358 > this._bogusPartTranslation[aPartName] = [fallbackPartName, parentName, parentPart]; you can wrap this in parens and just return that rather than listifying a second time. aka, "return (a = b);" is the same as "a = b; return a;" on file: mailnews/db/gloda/components/jsmimeemitter.js line 382 > if (childIndex in parentPart.parts) { please do a length check here; the semantics are different even if we aren't abusing them.

Kaie, we currently have no tests for s/mime decryption in Thunderbird, and I believe now would be a good opportunity to add at least one. Do you think you could whip up a quick test that: 1. creates a self-signed certificate, 2. adds it both as my identity's certificate and as a root certificate, 3. creates a message encoded with that certificate, 4. injects the encoded message into a folder, 5. streams it through the JS Mime Emitter, 6. ensures that everything went well (i.e. the message is properly decoded). Points 1., and 3. could be done beforehand, i.e. we could have a readymade certificate (hopefully that never expires) bundled with the test, and a readymade message as well. If you're not familiar with the test structure, and assuming you can write a test that at least does 1., 2. and 3., I could take it on from there and finish the work. Let me know how that works for you! I know absolutely nothing about s/mime, the mere procedure of setting up (manually, not even in a test) a test certificate was enough to scare me off it, so I'd be really glad if you could help me out with the first three items.

Attached is a stripped-down version of my fakeserver-ssl patch which should be sufficient to describe how to deal with certificates in an xpcshell test. I added some of this stuff with the mind that S/MIME might need to test this, so reusing these certificates is probably a good idea where possible.

I should point out: in this patch (unless I accidentally took it out), I have: * A root certificate * A signed certificate (These are generated for 5 or 10 years, I think) * A certificate database which should already mark everything as trusted for S/MIME * A function which loads this stuff (importCACertificates, especially if called with ../../../data/certs/cacerts.pem as the file -- this also shuts up dialog boxes on import, so it's better than trying to figure it out yourself) * Instructions for how to create more certificates and CA certificates for those who don't want to decode the NSS documentation importCACertificates is currently stuffed into ssld.js, although it probably ought to be moved to mailTestUtils or something.

Since this is your code, you're probably the most qualified person to separate the right functions into utility functions, and maybe extend the test file with functions to encrypt an email, and so on; what you uploaded only does the very first two steps of the outline from comment #12. Any chance you could do that while you're at it? That would make sure there's a set of neatly-designed functions to handle certs as part of the test suite. I don't think I would do something very clean / well-designed if I were to do it...

===== Testing S/MIME ===== So I did the right thing and introduced testing for S/MIME in our testing infrastructure. I went for the Mozmill route because I figured out we might want to test some UI features of S/MIME while we're at it. In order to make that possible, I added: - three files kindly provided by kaie into mailnews/test/data/db-tinderbox-invalid: these are key3.db, cert8.db and secmod.db, i.e. enough to have a profile whose cert DB is preloaded with a fake certificate registered in the name of "tinderbox@invalid.com", which expires in such a long time that I frankly hope Thunderbird will be replaced by something cooler then, - a wrapper.py script in mail/test/mozmill/folder-display-helpers.js that copies the aforementioned files into the profile folder before starting Thunderbird, - a test file called test-folder-display/test-display-smime.js that contains a big blurb of encrypted base64 data. Here's how I generated the blurb: - I took my test profile, and put the three .db files in it, - I changed one of my test addresses to be tinderbox@invalid.com, - I setup the account to use the certs created by kaie, - wrote an email using that account and set it to encrypt its content via S/MIME, - sent the email, - opened my sent folder, and viewed the sent message source. Advantages of this approach: - simple, - easy to add tests for more messages, - efficient, - doesn't require to write a zillion tests before we're able to even start displaying S/MIME messages. Drawbacks of this approach: - can't generate S/MIME messages in the tests (which means I can't add tests to mailnews/db/gloda/tests/test_mime_emitter.js), - doesn't test the UI for setting up an S/MIME account, - doesn't test the UI for sending with an S/MIME account. Frankly, it's easy to add tests for the last two bits, since now anyone can recycle my wrapper.py script and start writing tests for the S/MIME features. I'm hoping that someone will take that as an opportunity to write the extra tests to cover more S/MIME features. ===== JS Mime Emitter and crazy messages ====== Here's a technical discussion for the fix. - Encrypted messages - So it turns out encrypted parts emit a completely hallucinating MIME structure. Here's what libmime pretends is happening: root part: MimeMessage part 1: something that has type application/pkcs7-mime → MimeUnknown part 1.1: doesn't notify us about it part 1.1.something: the rest of the inner mime structure that was in the base64 blob → MimeContainer, MimeBody, MimeAttachment, etc. The exact same thing happens for PGP/MIME messages. Before the patch, we would simply drop any part labeled 1.1.X because it wouldn't have a parent (libmime not telling us about the 1.1 part). This is bad, because we're missing out information. So I added a first strategy to cope with these crazy messages: [Strategy 1] If a part doesn't have a parent, create a multipart/fake MimeContainer to serve as its parent. - UUEncoded attachments - UUEncoded attachments are not wrapped in MIME parts, so while we're in part 1 (MimeBody), we receive somehow a notification about part 1.2. Because we can't attach a MimeAttachment to a MimeBody (MimeBody is not a container), I added another strategy: [Strategy 2] If a part cannot be attached to its parent, walk up the MIME tree until we find a container we can attach ourselves to. UUEncoded attachments are crazy and I'm not too inclined to put much more energy into this, but I'm killing two stones with one bird here, so well... ===== Making sure MimeMsg drops encrypted parts ===== I added a line into mimecryp.cpp to emit an extra notification after we close the headers of the encrypted parts, in the style of the x-jsemitter-part-path notification, so that the JS Mime Emitter sets the isEncrypted bit on that part to 1; then, before passing the resulting MimeMsg to the consumer, we strip encrypted parts unless explicitly requested. The author wishes to thank Kaie for kindly providing the .db files ;-).

I just added an isEncrypted attribute to GlodaMessages because that's important for Thunderbird Conversations.

Comment on attachment 551185 [details] [diff] [review] Bigger patch the "if (childIndex in parentPart.parts) {" still needs to be changed to a length check and not be an "in" check. The gloda "isEncrypted" attribute should be singular, unless I'm missing something. Accordingly the empty set being significant is not relevant. folder-display is not a good mozmill directory for this; I suggest creating a "crypto" or "mime" subdirectory to convey that this is about streaming mime stuff for crypto purposes that requires its own profile. Accordingly, please also update wrapper.py to include an accurate comment inside rather than claiming to be a folder tree mode. The emitter check for "x-jsemitter-encrypted" in the emitter (processor) should check the value. We don't use waitForEval anymore, or won't very shortly, depending on when sid0's patch lands and we fully convert to the new mozmill, so be aware of that.

This patches addresses all the review comments. I'll do a try run of this patch, the one it depends on, and a third one, and I'll wait for sid0's feedback before committing them all.

Here's a try-run for this bug, bug 564423 and bug 655536 combined. - all green on linux64, osx, osx64, - and I think the failures on linux are unrelated; - however, the new directory I added in mailnews/test/data makes the build system unhappy on windows... http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=3777de03d4a9

Ok, rather than fiddling with makefiles, which I'm notoriously bad at, according to :Callek, I might just get away with not creating a subdir of mailnews/test/data/, which I think I'll do.

(In reply to Jonathan Protzenko [:protz] from comment #21) > Ok, rather than fiddling with makefiles, which I'm notoriously bad at, > according to :Callek, I might just get away with not creating a subdir of > mailnews/test/data/, which I think I'll do. Actually after speaking with sid0, turns out what I told you was partially wrong, and DIR_INSTALL works to correct the error. The try error (on windows) was: |nsinstall: e:\buildbot\try-w32\build\mailnews\test\data\db-tinderbox-invalid is a directory| and the command was using nsinstall.exe rather then nsinstall.py so somehow your DIR_INSTALL from the first (visible) attachment here was either not the only place we install it, or never made it to try. [last option and less-likely is it never regenerated the Makefile, but try is meant to be clobber-always iirc]

(In reply to Jonathan Protzenko [:protz] from comment #21) > Ok, rather than fiddling with makefiles, which I'm notoriously bad at, > according to :Callek, I might just get away with not creating a subdir of > mailnews/test/data/, which I think I'll do. You can use DIR_INSTALL to install directories over. Your try builds errored out was that you weren't using it. Do you want to stay with this approach or do you want to put an updated patch up?

s/was that/because

I'd rather keep the db files in separate directories, then. Can you be more specific with that DIR_INSTALL stuff? i.e. which Makefile to edit and where. Thanks.

Just use $(DIR_INSTALL) <directory> <destination> instead of $(INSTALL) <directory> <destination>.

Thanks. I've sent a new push to try. Is this a known issue that make check should be failing on linux?

Out of curiosity, why is this a MozMill test and not an xpcshell one?

Because I figured out that would make a good starting point for writing more s/mime related tests, esp. concerning the UI. But yes, I don't think there's a strong reason for that being a mozmill test except "hey we could use that to write more tests". Also I think I expected I would have to click various menu items to make the certs "right", and that would be easier via mozmill. In the end, it turned out I didn't have to. Also, you told me how to use wrapper.py to make the cert files be there right from the start, and I don't know the equivalent for xpcshell.