Closed Bug 1705765 Opened 2 years ago Closed 2 years ago

unable to create IMAP folders containing & (ampersand)

Categories

(MailNews Core :: Networking: IMAP, defect)

Thunderbird 89
defect

Tracking

(thunderbird_esr78 unaffected, thunderbird89+ fixed)

RESOLVED FIXED
90 Branch
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird89 + fixed

People

(Reporter: phoenixuk06, Assigned: gds)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

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

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?

Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All

There is still Bug 1690403 pending

See Also: → 1690403
Attached patch fix-lack-of-copy-to-mutf7.diff (obsolete) — Splinter Review

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.

Assignee: nobody → gds

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.

Attachment #9216619 - Attachment is obsolete: true

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.

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.

Attachment #9216681 - Attachment is obsolete: true

(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.

Attached patch Bug1705765-proposed-fix.patch (obsolete) — Splinter Review

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

Attachment #9216936 - Attachment is obsolete: true
Attachment #9216967 - Flags: review?(benc)
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.
Attachment #9216967 - Flags: review?(benc) → review+
Attached patch Bug1705765-proposed-fix-v2.patch (obsolete) — Splinter Review

Fixed the conditionals by adding braces.

Attachment #9216967 - Attachment is obsolete: true
Attachment #9217500 - Flags: review?(benc)
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;
}

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

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

Attachment #9217528 - Flags: review?(benc)
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)
Attachment #9217528 - Flags: review?(benc) → review+
Attachment #9217500 - Flags: review?(benc)
Attachment #9217500 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Target Milestone: --- → 90 Branch

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6e975cb9f460
Fix regression to allow creation and rename of imap folders containing ascii ampersand. r=benc

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

This is a regression and should go into beta.

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)
Attachment #9217528 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

Attachment #9217528 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.