1616114 - skiptrash in browser.messages.delete allows silent delete of mailbox messages

Status: RESOLVED FIXED

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

Description

[klaus]

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

Steps to reproduce:

maybe have an addon trash where these messages go? So that the user might be able to recover them?

Some of the 'security restrictions' for addons truely feel silly, but this feels like a serious risk. I would not have an addon delete my full mailbox - even if only by accident and putting a bad loop end into its code.

There are valid use cases for skiptrash, so maybe an addon specific trash would be good, which the user can clear if the addon works fine.

Comment 1

[klaus]

as o.marce points out, the corresponding permission is called messageMove which is totally hiding the desastrous delete option.

Would the current implementation call for a user confirmation?

Comment 2

[klaus]

as proof of concept, I wrote a mailextension addon that searches emails tagged 'important' and silently deletes them (finally).

According to https://groups.google.com/forum/#!topic/tb-planning/o10JBTAj2AU in planning, these addons are autoapproved. This function (skipTrash) can delete a full mailbox, unnoticed by the user.

The permission messagesMove totally hides how destructive this can be. Maybe create a new permission messagesDelete. I can see several use scenarios where it may be useful, but the end user must know what he is allowing.

Klaus

Comment 3

[klaus]

same probably applies to browser.folders.delete(). For the end user, permission accountsFolders would not imply allowance to delete.

I see remove duplicates, for example, needing delete on messages, but still, it needs to be transparent to the end user.

Comment 4

[klaus]

if I set fromMe to true in browser.messages.query, I can delete all the emails the user ever sent in one go in 68. He doesn't have to confirm.

According to the planning post, it would be autoapproved in 68.

Comment 5

[Daniel Veditz [:dveditz]]

This bug doesn't need to be hidden because the danger is exactly what the skiptrash option is documented as doing. Additionally the tradeoffs involved in the "post-review" extension process has been argued in public.

Personally I'm sympathetic to this reporter's point: do you need to support skiptrash at all? Should it require a separate messagesDelete permission? Flag extensions for higher scrutiny? I can also imagine those answers going against the reporter: extensions that move mail around can already cause havoc even without deleting them, install only from trusted authors!

Bugzilla isn't the best forum for that kind of discussion, but I don't know where the right place for Tbird discussion is these days. The tb-planning forum linked above, probably.

Comment 6

[John Bieling]

The end user is not seeing the permission string messagesMove, but the description "Move, copy, or delete your email messages". So the user has agreed to give those rights to the extensions (the argument that users do not read that is invalid, they should). If an extension is indeed ill-intended, users can report them and they get banned. This has proven to work for FF and reduced the review time of add-ons dramatically.

So from the permission point of few this is not a bug.

I do see a potential need for a more fine tuned delete permission. But there is a pitfall: The user can configure TB to delete messages directly instead of moving them to trash. In this case skiptrash is not needed to permanently delete messages. So a newly introduced permission which controls the usage of skiptrash can still not protect the user from permanent deletions. Even if we introduce messagesDelete (controls usage of messages.delete()) and messagesDelete.permanent (controls usage of skiptrash), the user is not safe from permanent deletion by not using add-ons who request messagesDelete.permanent.

Introducing a new permission which grants usage of messages.delete() is also problematic, as the developer can just use messages.move() to move messages to the trash folder (and auto delete will remove them later, also maybe unnoticed by the user).

I only see one way to improve the situation: Change the description of messagesMove to "Move, copy, or (permanently) delete your email messages". But I have to check if that is possible, because IIRC we cannot change the meaning of locale strings (just fix spelling errors), because a changed meaning needs to trigger updates of all languages, but only new locales get send to translators, not updates. I have to check.

Comment 7

[John Bieling]

After looking a bit longer, I think the effort needed to change the description is beyond reason. This is due to dependencies in toolkit code and the way translation updates are triggered. I would need to decouple the key of the permission's description from the name of the permission. Firefox needs to accept and do the same changes, as it is part of the shared code base.

What we could do: Find a name of a new permission, which indicates, that the add-on can override or ignore the default delete behavior. The important part is, that that permission should not just state that it allows for permanent deletions, but that it allows the add-on to ignore the current delete setting and skip the trash.

Any catchy ideas?

Comment 8

[klaus]

hmmmmm. What is the use case for skipping the trash? Why not just always delete into trash? Delete without asking for ok may be needed/handy, but does it need to be permanent?

I agree that autodelete may be a problem, but that is set by the user. If he switches off the second layer of protection, well, it is ... him/her.

xpiLib reports one addon using skipTrash (I didn't verify - at least it is a function parameter: async function deleteWithPriorityCheck(skipTrash) {

in xall/x68/4454-priority-switcher/src/scripts/background.js

Comment 9

[John Bieling]

Attachment: Bug 1616114 - Change permission descriptions to better describe their scope and add messagesDelete permission. r=darktrojan

Comment 10

[John Bieling]

If the skiptrash permission is denied, we could change the permission description itself to make it a bit more obvious. For that we need to make this fixed map configurable:
https://searchfox.org/comm-central/rev/960f62413739780001f44860259b86428a3d6664/mozilla/toolkit/components/extensions/Extension.jsm#1690

One idea is to move the mapping to ExtensionsUI.jsm, so m-c and c-c could have their own mappings.
See bug 1717201.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3802cb782609
Change permission descriptions to better describe their scope and add messagesDelete permission. r=darktrojan

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/906299529e1f
follow-up - Fix broken tests. rs=bustage-fix