Closed Bug 1588391 Opened 4 months ago Closed 4 months ago

Folders with space are not found when passed to various APIs

Categories

(Thunderbird :: Add-Ons: Extensions API, defect, P1)

defect

Tracking

(thunderbird_esr6870+ fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 70+ fixed
thunderbird71 --- fixed

People

(Reporter: Fallen, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

STR:

  1. Create a folder with a space in it (/INBOX/with space for example)
  2. Do something like this:
let folder = await messenger.accounts.get("account1").folders.find(folder => folder.name == "with space");
await messenger.mailTabs.update({ displayedFolder: folder });

Result:
Folder "/INBOX/with space" for account "account1" not found.

Expected:
Folder is correctly selected.

Addl Info:
The API encodes the folder URI to imap://me%40example.com@imap.gmail.com/INBOX/with%20space while the real folder uri is imap://me%40example.com@imap.gmail.com/INBOX/with space. The folder lookup service expects the folder path to be a string exact match to the folder uri, not an uri match. imap folders don't seem to escape spaces to %20, so the folder is not found.

I'm sensing there may be other edge cases, and it might even depend on the protocol type. When fixing this bug we should find out how to make sure we always use the correct path.

I'm marking this P1 since it breaks a bunch of APIs that work with folders (go to folder, copy/move messages to folders, etc). Geoff, do you have cycles to look into this?

Flags: needinfo?(geoff)

Space appears to be encoded in the URLs of POP or local folders but not IMAP folders. Can we be sure this is the case for all IMAP accounts?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(jorgk)
Flags: needinfo?(geoff)

Can you show us some examples. Going through my e-mail archive I found
imap://info%40hostalpancheta%2Ees@mail.hostalpancheta.es:993/fetch%3EUID%3E.INBOX.Jordi%20Knobloch%3E139

So clearly the space is encoded as %20.

Flags: needinfo?(jorgk)

Seems to me too we are encoding spaces, and that we should do so.

Flags: needinfo?(mkmelin+mozilla)

But we're not always encoding spaces, e.g. these two folders I just created:

mailbox://nobody@Local%20Folders/Trash/foo%20bar
imap://user@localhost/INBOX/foo bar

The question is, if we change the WE code to handle this based on the server being IMAP, is it going to work for all IMAP servers?

On the other hand if we change the IMAP code to always encode spaces, that has potential to break many things.

Where did you get that imap: URL from? Mine is from some debug in the bowels of the backend. And can is imap://user@localhost/INBOX/foo bar a real thing? Or did you substitute the real server name with localhost.

I'm writing a test. That's where the URL is from.

But to answer my own question, I've just created a folder called "this is a test" in my GMail account, loaded it, and gFolderDisplay.displayedFolder.folderURL has encoded spaces. But MailServices.folderLookup.getFolderForURL with encoded spaces doesn't find it. This is messy.

For folder lookup issues consult your fellow compatriot from the island further south ;-)

Huh. The folderURL and URI properties of a folder differ. One encodes the spaces, the other does too, but not for IMAP.

This seems wrong but it works.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9101010 - Flags: review?(mkmelin+mozilla)
Attachment #9101010 - Flags: feedback?(benc)
Comment on attachment 9101010 [details] [diff] [review]
1588391-folders-with-spaces-1.diff

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

Looks ok. I wonder if this wasn't also broken for IMAP servers using . or something else as separator... (unusual, but exists). 
I do think we should fix the folderURL and URI discrepancy, but let's do that elsewhere.
Attachment #9101010 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9101010 [details] [diff] [review]
1588391-folders-with-spaces-1.diff

I guess you want this uplifted.
Attachment #9101010 - Flags: approval-comm-esr68+
Attachment #9101010 - Flags: approval-comm-beta+

Yeah, but I want to see if Ben has anything insightful to say about it.

Comment on attachment 9101010 [details] [diff] [review]
1588391-folders-with-spaces-1.diff

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

::: mail/components/extensions/parent/ext-mail.js
@@ +1255,5 @@
>    if (path == "/") {
>      return rootURI;
>    }
> +  if (server.type == "imap") {
> +    return rootURI + path;

Maybe some comment here would be useful to explain why we're doing that, together with an example.

(In reply to Geoff Lankow (:darktrojan) from comment #12)

Yeah, but I want to see if Ben has anything insightful to say about it.

OK, I won't land it then. But we'll be building TB 70 beta 4 today or tomorrow.

Target Milestone: --- → Thunderbird 71.0

Now with a comment.

Attachment #9101010 - Attachment is obsolete: true
Attachment #9101010 - Flags: feedback?(benc)
Attachment #9101200 - Flags: review+
Attachment #9101200 - Flags: feedback?(benc)

Maybe it makes sense to normalize the URI in https://searchfox.org/comm-central/rev/364e5ba7492c8a13e104662a7e43769819e339f7/mailnews/base/src/folderLookupService.js#118 instead? This way you could pass whatever you want and the lookup would find it.

Flags: needinfo?(acelists)

I'm still confused. Shouldn't the (folder)URI always be percent-encoded? Why should we need to cater for "whatever you want"?

I do think we should fix the folderURL and URI discrepancy, but let's do that elsewhere.

I agree with that.

Firstly, if Geoff's patch fixes the problem and doesn't break anything, then I say we run with it for now.

It's a symptom of not being 100% clear on the naming/encoding rules in various places. And complicated by the fact that there are names already in the wild we need to continue supporting (eg URIs in virtualfolders.dat).

In this particular case:

What is folderURL? How does it differ to the folder's URI? The comments in the IDL don't help much...

From looking at the implementations, I'm guessing it's for retrieving a URL which you can actually open and request data from?

  • For IMAP, folderURL is an escaped version of URI ("imap:" protocol).
  • For local folders, folderURL is based on a URL pointing to the filesystem (but with "mailbox:" replacing "file:")
  • For news folders, it returns a "news:" or "snews:" url, using the hostname, port and folder name (with some escaping).

(if anyone can confirm this for me, I'll add some comments to the folderURL definition in the IDL file!)

For folder URIs in particular we need to do two things:

  1. decide on an canonical encoding/escaping scheme for folder URIs.
    My naive suggestion, under "Folder URIs": https://bugzilla.mozilla.org/show_bug.cgi?id=124287#c26 (all utf8, individual components urlencoded).
    I'm sure there's a proper W3C URI spec somewhere that covers this stuff :-)
    After removing RDF, maybe they're not strictly URIs any more? Just TB-specific folder IDs.

  2. figure out how to standardise the URI scheme without breaking existing code and data.
    eg: the folder lookup service could use more tolerant matching, eg:
    "imap:INBOX/foo bar" should equate to "imap:INBOX/foo%20bar" (but not "imap:INBOX%2Ffoo%20bar").

Flags: needinfo?(acelists)
Flags: needinfo?(acelists)
Attachment #9101200 - Flags: feedback?(benc) → feedback+
Keywords: checkin-needed
Attachment #9101200 - Flags: approval-comm-esr68?
Attachment #9101200 - Flags: approval-comm-beta?

I just wrote up URI normalisation in folder-lookup-service as Bug 1588944.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/7fc4bc60717a
In WebExtensions folder lookup, don't encode path components for IMAP servers; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 9101200 [details] [diff] [review]
1588391-folders-with-spaces-2.diff

I don't think there'll be another TB 70 beta.
Attachment #9101200 - Flags: approval-comm-esr68? → approval-comm-esr68+
Comment on attachment 9101200 [details] [diff] [review]
1588391-folders-with-spaces-2.diff

Missed TB 70 beta 4.
Attachment #9101200 - Flags: approval-comm-beta?
Attachment #9101010 - Flags: approval-comm-beta+
Flags: needinfo?(acelists)
Duplicate of this bug: 1600826
You need to log in before you can comment on or make changes to this bug.