1437282 - Cannot move a mail through drag and drop is subject contains invalid RFC2047 token (additional ==== padding)

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[tk1]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180128191252

Steps to reproduce:

- Received a spam mail with a "wierd" subject (see below)
- Message is filtered to Spam folder
- Want to move it with drag and drop to another folder

The subject of the mail is:
=?UTF-8?B?S2VpbmUgQW56YWhsdW5nIG7DtnRpZyArIOKCrCAxNDAgV2lsbGtvbW1lbnNib251cw=======?=

In the mail list, the subject is shown as empty.
Sending a mail to yourself reproduces the behavior.


Actual results:

- Moving is not possible: clicking and holding the mail in the list does not "pick it up" (any other mail clicked shows the mail following the mouse), therefore can't move to intended folder

- This also impacts selections: when the mail in question is included in the selection, the complete selection cannot be moved.

Please note: moving the mail through right clicking it works.


Expected results:

Regular behavior of being able to move the mail.

Comment 1

[Jorg K (CEST = GMT+2)]

The subject is RFC2047 encoded, decoded it says" Keine Anzahlung nötig + ? 140 Willkommensbonus".

Can you save the message to an .eml file and attach it here via "Attach File". Looks like SPAM, so it won't have any personal information. Then we can experiment.

Comment 2

[tk1]

I would like to, but it is not possible to 'save as' the message: clicking on it just doesn't do anything. Also double clicking the mail only opens a new tab, but nothing is displayed (other than in the preview pane).

I get the feeling that the reason is in the header, because I can reliably reproduce the behavior by sending an email to myself with the exact subject as included above (i.e. from first = till the last =).

The subject is btw not displayed as what it should be, that's why I wrote 'wierd' above. Other mails with RFC2047 encoded subject are displayed correctly.

Comment 3

[Jorg K (CEST = GMT+2)]

(In reply to tk1 from comment #2)
> I would like to, but it is not possible to 'save as' the message: clicking
> on it just doesn't do anything.
OK, create a new local folder, copy the message to it (via right-click), then attacht the entire mailbox file, it's just text.

Comment 4

[Alfred Peters [:infofrommozilla]]

Attachment: Subject with irregular MIME word end: '=======?='

I can confirm the bug.

Error console:
Error: Failed to decode base64 string!  jsmime.js:72:11

<https://dxr.mozilla.org/comm-central/rev/42bf9c98f0865fed58eaab2f753b88a627c3c9b7/mailnews/mime/jsmime/jsmime.js#72>

Comment 5

[Alfred Peters [:infofrommozilla]]

Attachment: Subject with irregular MIME word end: '=======?='

I can confirm the bug.

Error console:
Error: Failed to decode base64 string!  jsmime.js:72:11

<https://dxr.mozilla.org/comm-central/rev/42bf9c98f0865fed58eaab2f753b88a627c3c9b7/mailnews/mime/jsmime/jsmime.js#72>

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks Alfred, I was too lazy to put an example together myself :-(
The error seems to come from
https://dxr.mozilla.org/mozilla-central/rev/a8e153c55eeee93a11e87d325fb20c644421036f/js/xpconnect/src/nsXPConnect.cpp#973
Should we replace
  return [atob(sanitize), buffer];
with
  let decoded;
  try {
   decoded = atob(sanitize);
  catch (ex) {
   decoded = ""; // or some other string? like decoded =  sanitize;
  }
  return [decoded, buffer];

Or do you have a better suggestion. We already made the thing a little more robust in bug 1301989.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1437282-jsmime-atob.patch (v1)

This works, or do you have a better idea?

Comment 8

[Alfred Peters [:infofrommozilla]]

(In reply to Jorg K (GMT+1) from comment #6)
> Should we replace
>   return [atob(sanitize), buffer];
> with
>   let decoded;
>   try {
>    decoded = atob(sanitize);
>   catch (ex) {
>    decoded = ""; // or some other string? like decoded =  sanitize;
>   }
>   return [decoded, buffer];

That would take a measure against such flaws. But we should log a error message, otherwise we are searching at the next fault eternally.

> Or do you have a better suggestion. We already made the thing a little more
> robust in bug 1301989.

   sanitize = sanitize.substring(0, sanitize.length - excess);
+  // Delete all unnecessary '====' in padding
+  sanitize = sanitize.replace(/(====)+$/g, '');
   // Use the atob function we (ought to) have in global scope.
   return [atob(sanitize), buffer];

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1437282-jsmime-atob.patch (v2)

OK, I added your hunk and it works like a charme. I made it dual-authorship. Do you think we should leave the try/catch or just go with your solution?

Comment 10

[Alfred Peters [:infofrommozilla]]

Comment on attachment 8950108 [details] [diff] [review]
1437282-jsmime-atob.patch (v2)

Review of attachment 8950108 [details] [diff] [review]:
-----------------------------------------------------------------

Logging a bug works fine. And the correction of the MIME word as well.

Comment 11

[Alfred Peters [:infofrommozilla]]

(In reply to Jorg K (GMT+1) from comment #9)

> Do you think we should leave the try/catch

Yes, that's always good. Above all, it avoids the empty header.

Do we need a test? O:-)

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Alfred Peters from comment #11)
> Do we need a test? O:-)
OK, I'll add something for stripping the ====. It's harder for the catch case since reporting the error will be interpreted as a test failure (I believe).

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1437282-jsmime-atob.patch (v2) + test

Like this?

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1437282-jsmime-atob.patch (v2) + test

Grrr, trailing spaces.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1437282-jsmime-atob.patch (v3) + test

I've talked to Joshua on IRC about this. He didn't like the "non-standard JS" (Components.utils.reportError(ex)) in that library and he was of the opinion that the try/catch should go elsewhere at a higher level.

He also said to sanitise the input so that atob() wouldn't fail and to have tests for all the illegal cases we've considered.

So for now we're going with Alfred's hunk and the added test.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/932be9f10464
strip unnecessary ==== padding in base64 text. r=jorgk

Comment 17

[Jorg K (CEST = GMT+2)]

TB 59 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/61f79736679871419e4099822035d1bfe9254753

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8950427 [details] [diff] [review]
1437282-jsmime-atob.patch (v3) + test

It's not worth it at TB 52.7 when TB 60 ESR is coming out six weeks later.