848477 - Extensions are not allowed to use custom folders for mail storage

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Kent James (:rkent)]

As a regression from bug 750781, attempts by an extension (such as ExQuilla) to store mail for new account types in an alternate folder now generate an error message that prevents the account manager from being usable with that new account type. You get the error message:

The Local Directory path "C:\Users\Kent\AppData\Roaming\Thunderbird\Profiles\3gtxygma.default\ExQuilla\legacy.4emm-2.com" is not suitable for message storage. Please choose another directory.

Unfortunately the variables that need to be overridden for a new account type are private variables in function definitions that cannot be overridden by an overlay.

Comment 1

[:aceman]

I agree with your comment in bug 750781. I also do not like how this is hardcoded. But it is this way throughout the codebase. Can we get the subdir from the server with any attribute?
It looks like the info could be in the defaultLocalPath attribute of protocolInfo.

Comment 2

[Kent James (:rkent)]

At the least, the override mechanisms available for new core code should be as straightforward as possible to use by new account types.

In the current situation, the fix I would suggest is actually quite straightforward. All of the information needed to determine valid folders is in the data structure kDangerousDirs (which is a good design). Unfortunately that data structure is a private variable which is inaccessible to an overlay. Simply taking kDangerousDirs and elevating it to public would make this more easy to overlay.

As it turns out, the overlay I did to work around this is to replace checkDirectoryIsAllowed with a version that first checks for my valid directory, and if that is not found it calls the original function. But that is being done two levels higher than need be, which is inefficient, plus I am not able to use the existing data structure. Ideally I would just add "ExQuilla" (and "SkinkGlue" in my New Account Types addon) to safeSubdirs here:

  let kDangerousDirs = [
    { dirsvc: "ProfD", OS: null, safeSubdirs: [ "Mail", "ImapMail", "News" ] },

So I think the fix for this is going to be pretty simple.

I mainly though filed this to try to encourage coders and reviewers to pay more attention to extention-added account types in changes.

Comment 3

[WADA:World Anti-bad-Duping Agency]

Trick in mail directory creation under <profile>/Mail or ImapMail by default for known server type is;
> mail.root.imap-rel = [ProfD]ImapMail
> mail.root.pop3-rel = [ProfD]Mail
> mail.root.news-rel = [ProfD]News
> mail.root.none-rel = [ProfD]Mail
User can change this default location any time.

Is mail directory created via mail.root.pop3-rel = [ProfD]ExQuilla or [ProfD]../../../C:/MyDir/ExQuilla rejected by checkDirectoryIsAllowed?

Is it impossible to add this prefs for added server type?
If possible, "considering mail.root.<type>-rel as Safest" can be a practical sollution for many add-on developers and add-on users.

Comment 4

[:aceman]

WADA, I asked about those prefs when implementing the checks in bug 750781 comment 90 but got no response. I am not sure what their purpose is. But what happens if the user changes their value? Will e.g. [ProfD]Mail no longer be a safe folder if user changes mail.root.pop3-rel to something else? I don't think so.

But it seems the defaultLocalPath attribute of protocolInfo returns exactly this pref value...

rkent, would this be better for you? IM accounts define defaultLocalPath too, can ExQuilla do it too? Would it be enough of an universal solution ?

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #4)
> WADA, I asked about those prefs when implementing the checks in bug 750781 comment 90 but got no response.

Sorry, but I wasn't aware of you comment.

> I am not sure what their purpose is.
> But what happens if the user changes their value?

The pref is setting for default mail directory location.
For example,
If mail.root.pop3-rel=[ProfD]Mail, mail directory for newly created POP3 account with hostname=pop3.a.b.c becomes [ProfD]Mail/pop3.a.b.c, and if mail.root.pop3-rel=mail.root.pop3-rel=ProfD]../../../../../../../../C:/wada/MAIL-NEWS/MailXXX, mail directory for newly created POP3 account with hostname=pop3.a.b.c becomes C:\wada\MAIL-NEWS\MailXXX\pop3.a.b.c (on MS Win).
i.e. "Location of mail directory" and "Directory name for mail irectory" is never hard coded n Mozilla.

> Will e.g. [ProfD]Mail no longer be a safe folder if user changes mail.root.pop3-rel to something else?

It's all up to person who implemented logic of checkDirectoryIsAllowed, isn't it?
What can "user or addon developer who sets mail.root.<protocol>-rel for his convenience" do on logic of checkDirectoryIsAllowed created by someone?

Comment 6

[:aceman]

(In reply to WADA from comment #5)
> The pref is setting for default mail directory location.
> For example,
> If mail.root.pop3-rel=[ProfD]Mail, mail directory for newly created POP3
> account with hostname=pop3.a.b.c becomes [ProfD]Mail/pop3.a.b.c, and if
> mail.root.pop3-rel=mail.root.pop3-rel=ProfD]../../../../../../../../C:/wada/
> MAIL-NEWS/MailXXX, mail directory for newly created POP3 account with
> hostname=pop3.a.b.c becomes C:\wada\MAIL-NEWS\MailXXX\pop3.a.b.c (on MS Win).
> i.e. "Location of mail directory" and "Directory name for mail irectory" is
> never hard coded n Mozilla.
> 
> > Will e.g. [ProfD]Mail no longer be a safe folder if user changes mail.root.pop3-rel to something else?
> 
> It's all up to person who implemented logic of checkDirectoryIsAllowed,
> isn't it?
And that's me. That is why I ask if I can somehow use the mail.root.* prefs to make the checks more universal. I worry that the pref only has significance for NEW accounts but may not say anything about already existing ones.
What happens if the user sets it to "[ProfD]" or [ProfD]extensions ? We do not want to allow mail storage there... Or do we make that the user's responsibility? We could assume only knowledgeable users will touch those specific prefs.

> What can "user or addon developer who sets mail.root.<protocol>-rel for his
> convenience" do on logic of checkDirectoryIsAllowed created by someone?

I'll implement rkent's proposal but would also like to use those prefs if possible (so that extensions do not need to do anything new just for this check).

Comment 7

[:aceman]

Attachment: WIP patch

So if we could use the defaultLocalPath (which takes the mail.root. prefs into consideration), the patch could look like this.

Comment 8

[WADA:World Anti-bad-Duping Agency]

(In reply to :aceman from comment #6)
> to make the checks more universal. I worry that the pref only has
> significance for NEW accounts but may not say anything about already
> existing ones.

As you say, mail.root.xxx-rel is absolutely irrelevant to existent mail directory when setting is manually changed by user.

> What happens if the user sets it to "[ProfD]" or [ProfD]extensions ? 
>(snip)
> Or do we make that the user's responsibility?

I think that manual change of mail.root.xxx-rel by prefs.js editing, or by setting change via Config Editor, is always user's intention. If prefs.js is broken by user's intentional change, it's simply a "prefs.js corruption by user".
If such change is done by add-on developer, it's merely a fault of careless add-on developer.

> We do not want to allow mail storage there...

I think "safety check upon Server Settings/Directory Path: change via UI" is sufficient.

If you worry about "manual prefs.js corruption by user" case, I think considering "mail directory path of [ProfD] + non-null file path" as "not dangerous", considering "root.xxx-rel" as "safe", is sufficient. 
Anyone can remove [ProfD] part from mail.root.xxx-rel and mail.server.serverN.dir-rel in prefs.js, but no one can change string(path) obtained via "[ProfD]". 
So, I think [ProfD] can be used in safety check always.
- Top part of [ProfD]                  : not safe
- Sub directory of top part of [ProfD] : not safe
- [ProfD]                              : Dangerous
- default of root.xxx-rel==[ProfD]/yyy : Safe (yyy=Mail,ImapMail,News)
- [ProfD]/zzz                          : Safe, if zzz is not used by Tb
- [ProfD]/zzz/ ... /ppp                : Safe, if not used by Tb
- current root.xxx-rel                 : Safe
- Subdirectory of other mail directory : Can't be accepted
- Absolutely irrelevant to [ProfD]     : not dangerous
And, in case of this bug, ask user to use dangerous mail directory or not, and if Yes, sets serverN.dir-rel.bypass_safety_check=true.

Comment 9

[Kent James (:rkent)]

Comment on attachment 723272 [details] [diff] [review]
WIP patch

I did not review this patch, which seems to contain a number of changes unrelated to the simple requirement to make kDangerousDirs public, but the general approach is to just do that, which I approve. Do make any new public globals have the "g" prefix though.

As an extension writer, I also have no objection to adding some preference that allows extensions to specify safe directories for this check, or adding some equivalent parameter to either nsIMsgProtocolInfo or nsIMsgIncomingServer. But that approach to me seems overly complex to implement, as it forces you to iterate over all defined servers to do this check. I think I would prefer some sort of preference. Unfortunately the existing preference is not well defined for this purpose, something like:

mail.root.relativeDirectory.{type}

would have been preferable, with "type" == pop3, imap, or exquilla.

I hate to see already complex interfaces further complicated with parameters that are likely to be constant for a given account type, which is why I prefer a preference with a "type" subpreference over extending an idl.

Comment 10

[:aceman]

The changes in the patch are documenting the feature and because we already identified a potential pref that can be used for the automatic check. The mail.root.* prefs are returned via protocolInfo.defaultLocalPath attribute. So if we settle on that, we do not need to add new prefs/attributes.

Comment 11

[Kent James (:rkent)]

"The mail.root.* prefs are returned via protocolInfo.defaultLocalPath attribute"

OK fine, but that still requires you to iterate over all loaded servers to determine the list of possible types to do checkDirectoryIsAllowed, right?

I suppose that is OK. I wonder though if a list somewhere of valid types that could be extended would be worthwhile? Would there be any other users of such a list? If not, then perhaps iterating over the list of loaded servers to determine the valid list of types is OK.

Comment 12

[:aceman]

We do not need to iterate and to get any list of possible types. Do you mean to allow IMAP mail to be stored in ProfD/Mail instead of ProfD/ImapMail ? Yes, theoretically it could be allowed, but we do not do it for now. (Well actually with the new patch we do, if the user sets all mail.root.* to the same directory, he can do anything he wants. But then warranty is void :).)

The check works like this:
For the currentServer that is open in the account settings pane and its Local Directory path, we check if it is in any of the special dirs (including ProfD). If yes, we reject it. However for ProfD we have a special case, that if the path is under currentServer...protocolInfo.defaultLocalPath then we still consider it allowed.

So is your extension able to return .defaultLocalPath as "/path.to.profile/Mail/exQuilla" if we query .protocolInfo for type = "exQuilla" ?

Comment 13

[:aceman]

For the record, the check allows to place the local directory anywhere on the filesystem, just not into the special directories. And if it is under Profile, it must be in the special .defaultLocalPath subfolder.

Comment 14

[Kent James (:rkent)]

"So is your extension able to return .defaultLocalPath as "/path.to.profile/Mail/exQuilla" if we query .protocolInfo for type = "exQuilla" ?"

Yes - but I interpret "Mail" as the Local Folders root, so currently I use these paths:

/path.to.profile/ExQuilla and /path.to.profile/SkinkGlue

Comment 15

[:aceman]

Yes, sorry, it should be as you write, directly under Profile, not under Profile/Mail.

Mail is currently used by pop3, local folders, rss. It seems RSS does not even use the mail.root.* pref but returns .defaultLocalPath correctly.

Comment 16

[:aceman]

Attachment: patch v2

OK, so you can try this patch with the extension. Check if the defaultLocalPath is picked up correctly, so you do not need to overlay anything in the check.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 723643 [details] [diff] [review]
patch v2

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

Didn't try this, but looks ok to me. not a mailnews/ reviewer though

::: mailnews/base/prefs/content/AccountManager.js
@@ +319,2 @@
>     */
>    function checkDirectoryIsNotSpecial(aDirToCheck, aLocalPath) {

having Not in function names sure makes code hard to read :/

Comment 18

[Kent James (:rkent)]

I compiled this latest patch (under aurora since my extension has not yet been updated to Gecko 22), removed the fix from my extension that was forced by the bug 750781, and tested. The extension worked OK, so simply checking defaultLocalPath from the protocol info, as this patch does, solved the problem.

Comment 19

[Ian Neal]

Comment on attachment 723643 [details] [diff] [review]
patch v2

>+++ b/mailnews/base/prefs/content/AccountManager.js
>+/**
>+ * This array contains filesystem folders that are deemed unappropriate
Nit: inappropriate

>+   * @param aDirToCheck  An object describibing the directory to check to check.
Nit: describing and too many "to check"
>+   *        The object has the following members:
>+   *        dirsvc      : A path keyword to retrieve from the Directory service.
>+   *        dir         : An absolute filesystem path.
>+   *        OS          : A comma separated string defining on which Operating
>+   *                      systems the folder is unusable:
Perhaps should be "A string of comma separated values..."
>+   *                       null   = all
>+   *                       WINNT  = Windows
>+   *                       Darwin = OS X
>+   *                       Linux  = Linux
>+   *        safeSubdirs : An array of directory names that are
>+   *                      are allowed to be used under the tested directory.
>+   * @param aLocalPath  A nsIFile of the directory to check, intended for message storage.
"An" rather than "A"

>+  } // end of checkDirectoryIsNotSpecial
>+
>+  if (currentAccount.incomingServer) {
>+    let defaultPath = Components
>+      .classes["@mozilla.org/messenger/protocol/info;1?type=" +
>+               currentAccount.incomingServer.type]
>+      .getService(Components.interfaces.nsIMsgProtocolInfo)
>+      .defaultLocalPath;
>+    if (defaultPath) {
>+      defaultPath.normalize();
>+      if (defaultPath.contains(aLocalPath, true))
>+        return true;
>+    }
This should be probably commented and, for both the comment here and at the very top, is it worth mentioning that the preferred method is via defaultLocalPath?

r=me with those points addressed.

Comment 20

[:aceman]

Attachment: patch v3

Comment 21

[Kent James (:rkent)]

Comment on attachment 726792 [details] [diff] [review]
patch v3

Looks good, thanks for the patch.

Comment 22

[:aceman]

Thanks.

Comment 23

[:aceman]

I assume you need this on TB 21 too so that you do not need to have workarounds for that single version in your extensions.

Comment 24

[Kent James (:rkent)]

(In reply to :aceman from comment #23)
> I assume you need this on TB 21 too so that you do not need to have
> workarounds for that single version in your extensions.

I don't think this is really necessary, as I already have an ugly workaround, and version 21 will be completely obsolete soon enough.

Comment 25

[:aceman]

21 is only now going to beta.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2775bb72271a

Comment 27

[Mark Banner (:standard8)]

Not tracking per comment 24