100854 - creating folder uris on migration that aren't properly escaped

Status: VERIFIED FIXED

(MailNews Core :: Profile Migration, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

we're creating folder uris on migration that aren't properly escaped.

spun off from #91936

it looks like were creating uri's like "mailbox://nobody@Local Folders/Drafts"

basically, we have to make sure we escape the hostname and escape the folder 
name (david did this for "Unsent Messages" by changing the #define to "Unsent%
20Messages")

Comment 1

[scottputterman]

Does this happen to any migrated folder with a space in it?

Comment 2

[scottputterman]

reassigning to srilatha.

Comment 3

[scottputterman]

reassigning to cavin.

Comment 4

[Cavin Song]

Two things need to be taken care of:

1. 4.x escapes imap uri's in case folder names contain spaces and/or is a 
subfolder of another folder. For example: 

  "imap://qatest20@nsmail-2/Folders%2FJan%20Sent"

and it needs to be converted to:

  "imap://qatest20@nsmail-2/Folders/Jan Sent"

So we need to unescape the folder names. Note that 6.x doesn't escape spaces in 
an imap folder uri (but it does for local folder uri).

2. 4.x doesn't esacpe '/' and ' ' if the local folder names have hierarchy in it 
and/or contain spaces:

  "My Folders.sbd/New Drafts"

and it needs to be converted to:

  "My%20Folders/New%20Drafts"

So we need to get rid of all ".sbd" and then escape the name.

Comment 5

[Cavin Song]

A patch coming up. Changes were made to nsMessengerMigrator::Convert4XUri() and 
nsMessengerMigrator::SetSendLaterUriPref() to perform needed escaping and 
unescaping folder names depending on whether or not it's a local or imap uri.

Comment 6

[Cavin Song]

Attachment: proposed fix, v1

Comment 7

[Cavin Song]

Seth and David, can I get a review from you (since you worked on bug 91936 
before)? Thanks.

Comment 8

[(not reading, please use [e-mail] instead)]

it makes me nervous that local folder URIs are escaped, but IMAP folder URIs 
are not.

all URIs should be escaped UTF8.  it sounds like we have some underlying issues 
with our IMAP folder URIs.

question:  what uri do we put in prefs if I pick "imap://qatest20@nsmail-
2/Folders/Jan Sent" for my sent folder, from within mozilla?

I've also got comments on the patch, I'll attach them next.

Comment 9

[(not reading, please use [e-mail] instead)]

some comments on the patch:

1)

+    nsCAutoString unescaped_uri(old_uri);
+    nsUnescape(NS_CONST_CAST(char*, unescaped_uri.get()));

most of the time, if you find yourself doing NS_CONST_CAST, you're doing 
something wrong.
nsUnescape takes a char *, because it modifies the buffer.  you don't want to 
cast away
the constness of the CAutoString buffer.  In this case, I think you are going 
to want to
strdup the old_uri, pass that into nsUnescape, and free it later.

2)

+    // In 4.x if the local folder names have hierarchy in it then the uri is 
like:
+    //   "/My Folders.sbd/New Drafts"
+    // and it needs to be converted to:
+    //   "/My%20Folders/New%20Drafts"
+    // ie, we need to get rid of all ".sbd" then escape the spaces (can't 
escape '/').
+    nsCAutoString escaped_folderPath(folderPath);
+    PRInt32 offset;
+    while ((offset = escaped_folderPath.Find(".sbd")) != -1)
+      escaped_folderPath.Cut(offset, 4);

two edge case:  

a) what if my non-leaf folder name is "foo.sbd", so I 
have:  "/foo.sbd.sbd/Cheese"
b) what if my leaf folder name is "foo.sbd", so I have:  "/Cheese/foo.sbd"

do you want to look for ".sdb/" and replace that with "/"?  

also, remember to use kNotFound instead of -1

3)

+    escaped_folderPath.ReplaceSubstring(" ", "%20");

that looks like weak escaping.  do we have to worry about any other illegal 
characters?

Comment 10

[Cavin Song]

> ------- Additional Comment #8 From Seth Spitzer 2002-02-25 11:46
> question:  what uri do we put in prefs if I pick "imap://qatest20@nsmail-
> 2/Folders/Jan Sent" for my sent folder, from within mozilla?
>
Here is an example (tempplate folder is used in this case):

  user_pref("mail.identity.id1.stationery_folder",
            "imap://qatest20@nsmail-2/Test Folder/My Template");

Spaces are not escaped (leaf or non-leaf).

Comment 11

[Cavin Song]

> ------- Additional Comment #9 From Seth Spitzer 2002-02-25 11:52
> 1)
>
OK.

> 2)
>
Good catch. Thanks.

> 3)
>
The reason of doing weak escaping is that we don't want to turn '/' into "%2F" 
(same reason as indicated in bug 91936 comment #28).

Good question about escaping other illegal characters. I would think that we 
don't have to at this point because the illegal chars should have been taken of 
prior to the migration time (by 4.x). If we have to do something here that means 
we'll have to have platform specific code here as well. Let's discuss more if 
you have other concerns.

Comment 12

[Cavin Song]

On winnt, I just did a test and I think we'll have to escape char like '#'. In 
4.x I have the templates folder set to 'folder#_1' and the 4.x prefs.js entry 
looks like:

 user_pref("mail.default_templates", "mailbox:/C|/Program 
            Files/Netscape/Users/allimapsettings/mail/folder#_1");

After migration, 6.x prefs.js contains the following entry:

 user_pref("mail.identity.id2.stationery_folder",
           "mailbox://nobody@Local%20Folders/folder#_1");

At this point the setting of femplates folder (in Account Settngs|Copies & 
Folders) shows "Click here to select a folder" istead of "foder#_1 on Local 
Folders". If I manually change "foder#_1" to "foder%23_1" in prefs.js then the 
folder setting would display correctly.

So I think we can escape the whole uri and then unescape %2F (ie, replace "%2F" 
with "/"). I'm not sure if this will affect/break i18n though since I can't it 
on my machine.

Comment 13

[(not reading, please use [e-mail] instead)]

cavin, bienvenu and I discussed this over aim today.

basically, imap folder URIs are incorrect, but now is not a good time to fix 
them.  one of the reasons to not fix them yet, is when we do fix them, we'll 
have to take are of some internal pref migration of imap folder URIs (possibly 
internal filter imap folder URI migration? localstore.rdf issues?)

for now, we should proceed ahead as is, and leave the imap folder URIs as 
unescaped.  (we don't want to cause a lot of regressions or require a lot of 
additional work right before mozilla 1.0)

so for now, cavin's doing the right thing.  figure out how 4.x represented URIs 
(local and imap), see how 6.x is expecting the URIs (it turns out, mailbox 
folder URIs are escaped, imap folder URIS are not), and then fix the migration 
code to deal with this.

note, this affects some work bienvenu is doing for shared folders, but he's got 
that under control.

cavin's working on an updated patch for this problem.

a while back, this came up.  

see http://bugzilla.mozilla.org/show_bug.cgi?id=84045

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 70648 [details] [diff] [review]
proposed fix, v1

needs work, cavin is coming up with a new patch.

Comment 15

[Cavin Song]

Attachment: Incorporated comments from Comment #9

For local folders we need to get rid of all ".sbd" and then escape the ruis.

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 71789 [details] [diff] [review]
Incorporated comments from Comment #9

sr=sspitzer

one typ on in your comment:

"then escape the ruis"
should be
"then escape the URIs", right?

cavin, can you list all the various 4.x pref values that you tested for QA?

Comment 17

[Jean-Francois Ducarroz]

Comment on attachment 71789 [details] [diff] [review]
Incorporated comments from Comment #9

R=ducarroz

Comment 18

[Cavin Song]

To test the patch you can create a new 4.x imap account and change the folder 
settings as following:

1. Drafts folder: Set it to a subfolder of another imap folder. For example,
"New Drafts" under "My Folders". Check 4.x prefs.js and the setting should
be something like:

 "IMAP://qatest20@nsmail-2/My%20Folders%2FNew%20Drafts"

2. Sent folder: Set it to a subfolder of another local folder. For example,
"New Sent" under "My Folders". Check 4.x prefs.js and the setting should be
something like:

 "mailbox:/C|/Program Files/Netscape/Users/TestAcct/mail/My Folders.sbd/New 
Sent"

Then migrate the account and after migration finishes, log into the server and 
go to Account Settings|Copies and Folders and you should see that both Drafts 
and Sent folders are set correctly (instead of "Click here to select a folder"). 
Also check 6.x prefs.js and the settings for Drafts and Sent folder should be:

  "imap://qatest20@nsmail-2/My Folders/New Drafts"
  "mailbox://nobody@Local%20Folders/My%20Folders/New%20Sent"

Repeat step #1 but this time use a folder name with a '#' in it. For example, 
"New Drafts#1". Check 4.x prefs.js and the setting should be something like 
(note that '#" has been escaped as "%23"):

 "IMAP://qatest20@nsmail-2/My%20Folders%2FNew%20Drafts%231"

Repeat step #2 but use a different folder name with a '#'. For example, "New 
Drafts#1". Check 4.x prefs.js and the setting should be something like:

 "mailbox:/C|/Program Files/Netscape/Users/TestAcct/mail/My Folders.sbd/New 
Sent#1"

Then migrate the account and the 6.x prefs settings should be like:

 "imap://qatest20@nsmail-2/My Folders/New Drafts#1"
 "mailbox://nobody@Local%20Folders/My%20Folders/New%20Sent%231"

Go through the same procedure for a new 4.x pop account.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 71789 [details] [diff] [review]
Incorporated comments from Comment #9

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 20

[Cavin Song]

Fix checked into trunk.

Comment 21

[Sheela Ravindran]

Marking verified. Tested all the scenarios listed by Cavin in his comment #18
for QA.
Verified on win98 using 05-15-08 Branch build.  I am marking this as verified
because this was fixed a while ago in the trunk before we branched for 1.0.0