1644032 - wrong accountId if X-Account-Key inside header

Status: RESOLVED FIXED

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

Description

[plieser]

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

Steps to reproduce:

1. Create an email (as eml file) that contains the X-Account-Key: account40 header
2. Add this email to e.g. the local folder
3. Get the message via WebExtension: message = browser.messages.get(id)
4. Get the account ID for the message with accountId = message.folder.accountId

Actual results:

accountId is account40 instead of the correct value.

Expected results:

accountId should reflect the folder/account the message is actually in, not what is inside the X-Account-Key header in the mail.

Comment 1

[John Bieling (:TbSync)]

Attachment: bug1644032 - wrong accountID.zip

I cannot reproduce this with TB78.5.0. The add-on I was using for my test is attached.

I moved an email from TB to my desktop, changed the id of that email and added the extra header and then added it back to Thunderbird. It did not return account40 but account1.

The add-on ads a message_display_action button and it will log the accountId and the raw email of the currently selected/displayed message to the console.

Can you confirm?

Comment 2

[plieser]

Did some testing with your add-on, and it looks like only the "Lokal Folder" account is affected.

Comment 3

[plieser]

A POP account is also affected by this. Only the IMAP account I tested with is unaffected.

Comment 4

[John Bieling (:TbSync)]

Confirmed on TB78 and current Daily.

Comment 5

[John Bieling (:TbSync)]

The reason is in this code:
https://searchfox.org/comm-central/rev/7e8d6a200456b336bdc593c4bfc699ad077019ea/mail/components/extensions/parent/ext-mail.js#1514-1518

function convertFolder(folder, accountId) {
  if (!folder) {
    return null;
  }
  if (!accountId) {
    let server = folder.server;
    let account = MailServices.accounts.FindAccountForServer(server);
    accountId = account.key;
  }

For IMAP accounts the incomming accountId (which is msgHdr.accountKey) is always an empty string, regardless of the X-Account-Key header being defined or not. The final accountId will be retrieved by MailServices.accounts.FindAccountForServer.

For POP and LocalFolder folders, the incomming accountId is the value of the X-Account-Key header. MailServices.accounts.FindAccountForServer returns the correct accountId for these folders as well.

How to fix this?
Make the current fallback the default and use the incomming accountId as fallback?
Ignore the accountId from msgHdr.accountKey altogether and return null if FindAccountForServer fails?

Comment 6

[Geoff Lankow (:darktrojan)]

That's frustrating. The accountId argument is an optimisation, because FindAccountForServer is expensive and this function tends to be called many times at once (e.g. when getting 100 messages from a message list, it's called 100 times). But as you say, on IMAP it doesn't even work as intended so the expensive part is running anyway.

Let's just remove the accountId argument and run FindAccountForServer in all cases.

Comment 7

[John Bieling (:TbSync)]

Attachment: bug1644032_fix_accountId_in_folders_accounts_api.patch

Understood. I propose and did the following in the attached patch:

Looking at the code and possible performance issues, I did not remove accountId from convertFolder() but just no longer pass in the not helping msgHdr.accountKey. The FolderManager.convert() function can still accept an accountId, if that information is already available.

Furthermore, I noticed, that traverseSubfolders() is not accepting an accountId at all, even though subfolders should be from the same account and thus we do not need to rerun the expensive lookup. The method browser.accounts.list() is actually passing in the correct accountId.
https://searchfox.org/comm-central/rev/f8c11a36225bbe5e145f1a9a0a3801428e265715/mail/components/extensions/parent/ext-accounts.js#25

I therefore changed traverseSubfolders() to accept an accountId.

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189722 [details] [diff] [review]
bug1644032_fix_accountId_in_folders_accounts_api.patch

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

Good catch. We're already calling traverseSubfolders with a second argument in ext-accounts.js.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/508b9ca35fac
Ignore accountId from message headers when converting folders for WebExt APIs. r=darktrojan

Comment 10

[Geoff Lankow (:darktrojan)]

I added some comments to try to avoid making the same mistake again.

Comment 11

[John Bieling (:TbSync)]

c-b: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=deb52d0847ca3f7a77eac2bf61bf986f2d3f637a

Comment 12

[John Bieling (:TbSync)]

Comment on attachment 9189722 [details] [diff] [review]
bug1644032_fix_accountId_in_folders_accounts_api.patch

[Approval Request Comment]
User impact if declined:
WebExtension API remains broken

Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=deb52d0847ca3f7a77eac2bf61bf986f2d3f637a

Risk to taking this patch (and alternatives if risky):
I hope none.

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9189722 [details] [diff] [review]
bug1644032_fix_accountId_in_folders_accounts_api.patch

[Triage Comment]
Approved for beta

Comment 14

[Dave Miller [:justdave]]

Thunderbird 84.0b3
https://hg.mozilla.org/releases/comm-beta/rev/31646aae0d3f

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9189722 [details] [diff] [review]
bug1644032_fix_accountId_in_folders_accounts_api.patch

[Triage Comment]
Approved for esr78

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 78.6.0:
https://hg.mozilla.org/releases/comm-esr78/rev/f5a6e12a95a9