1705765 - unable to create IMAP folders containing & (ampersand)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[DRW]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:87.0) Gecko/20100101 Firefox/87.0

Steps to reproduce:

File -> New -> Folder
or
right click account -> New Folder

enter name containing &, click Create Folder

folders created via webmail are visible

Application Basics

Name: Thunderbird
Version: 89.0a1
Build ID: 20210416094903
Distribution ID:

Update Channel: nightly
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Thunderbird/89.0a1
OS: Windows_NT 10.0 19042

Launcher Process: Enabled
Multiprocess Windows: 0/0
Fission Windows: 0/0
          Disabled by safe mode
Remote Processes: 1
Enterprise Policies: Inactive
Google Location Service Key: Missing
Google Safebrowsing Key: Missing
Mozilla Location Service Key: Missing
Safe Mode: true

Actual results:

folder is not created (verified with webmail)

Expected results:

folder is created

Comment 1

[Alfred Peters [:infofrommozilla]]

I can confirm that there is something wrong with the MUTF-7 / UTF-8 handling.
I created a folder with the name "test&more" once with TB78.9.1 and once with current trunk build:

created with:	TB78	TB89
file in Profile:	test&-more.msf	test&-more.msf
folder properties location:	test%26-more	test%26more
folder properties name:	test&more	test骊
foldername in folderpane:	test&more	test骊
foldername created on IMAP server:	test&-more	test&more

Possibly a regression by Bug 1571672 or one of its regression fixes?

Comment 2

[Alfred Peters [:infofrommozilla]]

There is still Bug 1690403 pending

Comment 3

[gene smith]

Attachment: fix-lack-of-copy-to-mutf7.diff

I found the problem when the server is not UTF8=ACCEPT capable. In that case, as the reporter has observed, the server doesn't create the folder with "&" in it. On imap CREATE, the server reports that the mailbox name is not encoded with valid MUTF7.

The problem is because on folder create the string containing "&" is seen as ASCII so it is never converted to MUTF7. The attached diff fixes that.

However, there is still a problem with servers that do support UTF8=ACCEPT. For them the correct string is sent to create the folder and the server LISTs the correct folder string containing the "&". Also the profile folder name is correct and contain the "&" but the displayed folder name in the tree contains 3-byte UTF8 (e.g., "test&such" display as "test닧". Don't know where that is coming from.

Comment 4

[gene smith]

Attachment: fix-lack-of-copy-to-mutf7-v2.diff

This adds a fix for the 2nd problem to the diff:

However, there is still a problem with servers that do support UTF8=ACCEPT. For them the correct string is sent to create the folder and the server LISTs the correct folder string containing the &. Also the profile folder name is correct and contains the & but the displayed folder name in the tree contains 3-byte UTF8 (e.g., test&such display as test닧. Don't know where that is coming from.

The problem here is that for UTF8=ACCEPT servers the folder name may be plain ASCII but contains a ampersand without a following dash as is required for MUTF-7. This causes bad conversion to UTF-16 which is used to display the folder name in the tree resulting in unexpected UTF-8 char(s) appearing such astest닧.
The solution is to check that the source string is valid MUTF-7. I can't find something like Is_MUTF7(str) so I convert it to UTF16 and then back to MUTF7. If if converts back to the same string then I assume that the source string is valid MUTF-7 and the conversion to UTF-16 is correct. Otherwise, I treat the ASCII string as UTF-8 and convert it to UTF-16.

This a hack on a "hacky function" so probably not optimal, but it seems to fix this bug. Also, left in some printfs I used to check that this really works.

Note: To create a folder named this test&-less you must enter test&--less; otherwise the dash does not appear and you get test&less for the folder name.

Comment 5

[Alfred Peters [:infofrommozilla]]

Sounds reasonable.
Currently, unfortunately, I can not compile myself. If you want me to test the patch before checkin, you would have to initiate a Windows try build.
Otherwise, give it a try.

Comment 6

[gene smith]

Attachment: fix-lack-of-copy-to-mutf7-v3.diff

I also noticed that I needed the same fix for "rename" so this new diff includes it.

I did a "try" win64 build here as you requested:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0448a9fb117193cd269a0472eff131a85f51b904

It works OK for me but good to have someone else try it.
The v3 diff is applied to the current trunk to produce the build.

Comment 7

[Alfred Peters [:infofrommozilla]]

(In reply to gene smith from comment #6)

Created attachment 9216936 [details] [diff] [review]
fix-lack-of-copy-to-mutf7-v3.diff

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0448a9fb117193cd269a0472eff131a85f51b904

Works fine with my local non UTF8=ACCEPT IMAP server.

Comment 8

[gene smith]

Attachment: Bug1705765-proposed-fix.patch

Same as v3 diff but with printf's removed and clang format applied.

Comment 9

[Ben Campbell]

Comment on attachment 9216967 [details] [diff] [review]
Bug1705765-proposed-fix.patch

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

Looks good. An elegant hack to work around the existing awfulness :-)

::: mailnews/imap/src/nsImapService.cpp
@@ +2173,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (utf8AcceptEnabled)
> +        CopyUTF16toUTF8(newLeafName, utfNewName);
> +      else
> +        CopyUTF16toMUTF7(newLeafName, utfNewName);

nit: coding style requires braces on conditionals if they have an else (I had to look that up - it's part of the google C++ style guide the mozilla guide is based on. `mach clang-format` doesn't seem to check for it though...).

@@ +2228,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (utf8AcceptEnabled)
> +        CopyUTF16toUTF8(newFolderName, utfNewName);
> +      else
> +        CopyUTF16toMUTF7(newFolderName, utfNewName);

and here.

Comment 10

[gene smith]

Attachment: Bug1705765-proposed-fix-v2.patch

Fixed the conditionals by adding braces.

Comment 11

[José M. Muñoz]

Comment on attachment 9217500 [details] [diff] [review]
Bug1705765-proposed-fix-v2.patch

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

Some drive-by comments.

::: mailnews/base/src/nsMsgI18N.cpp
@@ +140,5 @@
>  // Hacky function to use for IMAP folders where the name can be in
>  // MUTF-7 or UTF-8.
>  nsresult CopyFolderNameToUTF16(const nsACString& aSrc, nsAString& aDest) {
>    if (NS_IsAscii(aSrc.BeginReading(), aSrc.Length())) {
> +    // An ascii string may not be valid MUTF-7. For example, it may contain an

Nit: ASCII as in line 154 of the patch.

@@ +145,5 @@
> +    // ampersand not immediately followed by a dash, which is invalid MUTF-7.
> +    // Check for validity by converting to UTF-16 and then back to MUTF-7 and
> +    // the result should be unchanged. If the MUTF-7 is invalid, treat it as
> +    // UTF-8.
> +    nsresult rv = CopyMUTF7toUTF16(aSrc, aDest);

The error looks strange here. CopyMUTF7toUTF16() always returns NS_OK. So you have two choices:

First: Do not look at the status, that would be:
CopyMUTF7toUTF16(aSrc, aDest);
nsAutoCString tmp;
CopyUTF16toMUTF7(aDest, tmp);
if (aSrc.Equals(tmp)) return NS_OK;

Second, look at the status and react accordingly, that would be:
nsresult rv = CopyMUTF7toUTF16(aSrc, aDest);
if (NS_SUCEEDED(rv)) {
  nsAutoCString tmp;
  CopyUTF16toMUTF7(aDest, tmp);
  if (aSrc.Equals(tmp)) return NS_OK;
}

Comment 12

[José M. Muñoz]

Or shorter for the second choice: if (NS_SUCEEDED(CopyMUTF7toUTF16(aSrc, aDest))) {

Comment 13

[gene smith]

Attachment: Bug1705765-proposed-fix-v3.patch

Updated per "Jose's" drive-by suggestion. I'll let Ben decide which version to go with or if it needs further tweaks.

Comment 14

[Ben Campbell]

Comment on attachment 9217528 [details] [diff] [review]
Bug1705765-proposed-fix-v3.patch

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

Looks great to me!
(btw: for any 'nit' feedback it's fine to keep any existing review in place - if you upload a new patch you can just set the r+ on it)

Comment 15

[José M. Muñoz]

Note that TB 78 is not affected. The Unicode stuff started in bug 1571672 (TB 83). You should backport to beta though.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6e975cb9f460
Fix regression to allow creation and rename of imap folders containing ascii ampersand. r=benc

Comment 17

[José M. Muñoz]

This is a regression and should go into beta.

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9217528 [details] [diff] [review]
Bug1705765-proposed-fix-v3.patch

[Approval Request Comment]
Regression caused by (bug #): unclear exactly which, some of the IMAP UTF-8 bugs
User impact if declined: problems with folders containing &
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): some, but not huge risk

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9217528 [details] [diff] [review]
Bug1705765-proposed-fix-v3.patch

[Triage Comment]
Approved for beta

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 89.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/28f7dd70c681