ProtonMail / proton-bridge

Proton Mail Bridge application
GNU General Public License v3.0
1.16k stars 157 forks source link

APPEND result when targetSeq.Len() == 0 #2

Closed exander77 closed 4 years ago

exander77 commented 4 years ago

Already reported to support. Adding here as well.

Is this really ok, to return success if the sequence is empty? Seems to cover failure on the server.

  info := appendSucess
  if targetSeq.Len() > 0 {
    info = fmt.Sprintf("[%s %d %s] %s",
      appenduid,
      uidValidity,
      targetSeq.String(),
      appendSucess,
    )
  }

  return &imap.StatusResp{
    Type: imap.StatusOk,
    Info: info,
  }

My report:

IMAP over ProtonBridge silently fails when importing mail into Sent folder where From corresponds to one of the user adresses causing a loss of emails.

Consider I have mail@radomirpolach.cz added to my account and minimal example below:

When I try to import a message like this where From corresponds to mail@mydomain.cz account: MIME-Version: 1.0 Date: Mon, 10 Jan 2021 12:12:12 +0200 Message-ID: test@example.com Subject: Test From: mail@mydomain.cz To: test@example.com

It results in this IMAP communication: 45:34.13 > b'KLII4 APPEND Sent "13-Apr-2020 01:45:34 +0200" {160}' 45:34.13 < b'+ send literal' 45:34.13 write literal size 160 45:34.13 < b'KLII4 OK APPEND completed'

Even though completed, the email is not added into the Sent folder. ProtonBridge does not report any error as well, just ordinary: No matching UID, continuing APPEND to Sent

When I on the other hand try to import a message where From corresponds to a different address not added to my account: MIME-Version: 1.0 Date: Mon, 10 Jan 2021 12:12:12 +0200 Message-ID: test@example.com Subject: Test From: mail2@mydomain.cz To: test@example.com

It results in this IMAP communication: 45:47.98 > b'OFJB4 APPEND Sent "13-Apr-2020 01:45:47 +0200" {161}' 45:47.98 < b'+ send literal' 45:47.98 write literal size 161 45:48.30 < b'OFJB4 OK [APPENDUID 4 33] APPEND successful'

And the email is stored in the Sent folder. ProtonBridge reports: Importing external message

This does not affect Inbox, only on Sent.

exander77 commented 4 years ago

But this really needs an assessment from guys from ProtonMail.

cuthix commented 4 years ago

There were plenty reports that users see duplicates in Sent mailbox so we decided to filter APPENDs into Sent mailbox when sender is identical to account address and silently sends OK without import. We have plan to improve this in meanwhile you can try to append to different folder than Sent

For more details you can check the code:
https://github.com/ProtonMail/proton-bridge/blob/master/internal/imap/mailbox_message.go#L113

cuthix commented 4 years ago

reference in GODT-143

exander77 commented 4 years ago

@cuthix That's a scary fix. Anybody who tries to move mails into Sent will lose them and probably won't even notice. It may be a solution for me to move into another folder, but it is not a solution at all in a general sense.

Please deploy a fix as soon as possible and if I may advise you: never return success when the action didn't actually succeed as this could have catastrophic side effects.

It destroyed almost 5 hundred of my emails which I needed to recover from backup.

If I didn't notice... and there are surely people who didn't. Continuing with this practice is extremely irresponsible.

exander77 commented 4 years ago

There were plenty reports that users see duplicates in Sent mailbox so we decided to filter APPENDs into Sent mailbox when sender is identical to account address and silently sends OK without import.

I am rereading this and I am really terrified, really terrified. From my perspective, it is like: "Some people had email duplicates, so We decided to shred some emails as a workaround." I thought it was a bug on the server, but this is a deliberate, intentional and malicious act from the Bridge.

It is as if you were a bunch of surgeons who found out that some people have developed a problematic third kidney and decided that the best way to tackle this problem would be to preemptively remove one kidney from all of us.

List of scenarios I can think of:

  1. This will lose emails directly when moved from a third party account to ProtonMail account.
  2. This will lose emails directly when moved from one ProtonMail account to another ProtonMail. account.
  3. This will lose emails indirectly when you copy them to Send and remove originals.
  4. This will lose emails indirectly when you move them to Send and find they are still in the original location and think you have copied them instead of moving them.

This is just from the top of my head. And I was actually affected by 3 of those 4 mentioned. And I know only because I have checked and rechecked several times to see everything is in order. I became extremely paranoid when I lost 5 hundred emails in the first 10 minutes I started migrating them from Gmail to ProtonMail. By pure luck the first batch of emails I moved was Sent folder from the primary mail I was moving where the From corresponded and not a single one made it through which put me on high alert. I may have not even noticed otherwise.

exander77 commented 4 years ago

I suggest fix for this to be deployed ASAP and for ProtonMail to inform all Bridge users not to use the affected versions. It is extremely easy to lose emails and don't even notice. I would even recommend advising users not to copy anything into Sent folder until this is fixed.

exander77 commented 4 years ago

I am starting to worry that nothing so far has been done with this issue which causes data loss.

d310n9 commented 4 years ago

TLDR: We bumped the priority on reevaluating this behavior and will try to find a better solution which prevents this scenario from happening at all in the coming sprints More details:

Thanks again for flagging this issue, apologies for the pain you've encountered. As per previous discussion we acknowledge this behavior is frustrating and can lead to message loss under the following conditions a/ when trying to import/append to sent folder, AND b/ when the sender of the imported emails = the email address of the (if either a/ or b/ does not apply, importing will work successfully)

To give you some background on why this was done in the first place: 1./ So far Bridge was primarily designed as an email processing client for sending & receiving mail, and not designed for importing. However now that Bridge has larger adoption, we do see the need for and want to provide users the ability to import reliably through it as well.

2./ For send receive use case: Outlook on Windows (which is a significant portion of email client userbase) has an interesting behaviour where, after sending a message and trying to append the recently sent message to the send folder (as other email clients) this appended message uses a different messageID than what was previously used during drafts and sending (outlook specific)

Long story short, if Bridge doesn't do anything, all Outlook users will see duplicate messages in the sent folder, which could erroneously be interpreted as "Bridge sent my message twice". Given that we prioritized send & receive over import workflow in the past, we built the feature as it works today. Going forward we'll be working on a solution which does both (i.e. functions as expected Outlook send but also allows users to successfully import to the sent folder for all cases). If you have some ideas, we'd be happy to discuss as well.

exander77 commented 4 years ago

@d310n9 Regarding Outlook, isn't this an intended behaviour for using 3rd party SMTP server (or in some other case you actually want both of these to compare what you created and what was actually sent)? New Outlook has an opt-in option "Save copies of messages in the Sent items folder" or in older versions, opt-out option "Do not save copies of sent items". Is this what you are talking about? If so, then you are actually breaking even the intended and configurable Outlook behaviour. I think the different Message-ID is there by design to prevent behaviour you are doing - merging sent message with its copy. They probably thought that this should 100% ensure that both messages would be present... and it should have, yet you broke it as well as "Save copies of messages in the Sent items folder" is clearly prevented to save a copy inside Sent...

Old: save-sent-items-imap New: save-sent-options

There are two ways an email can appear in the Sent folder. Either it is put there by an SMTP server which is connected to an email account or it is put there through IMAP APPEND into the Sent folder. Historically the first option was not usually the case as people used local SMTP servers of their internet providers or employer's internet providers etc. So, SMTP servers were usually completely separate from email accounts. These days a lot of users use their email provider's mail servers which are authorized and put sent emails into the Sent folder automatically. So there is usually no need to to put them there through IMAP Append into the Sent folder. If I understand correctly what you are doing, then you are trying to fix what is not broken, by breaking it and breaking other things as well.

I understand that the Bridge become more than it was, but I would point out that there lurks a bad practice. The software should not lie. It should not report success when it failed. It is ok if it fails for some corner cases, it may be unpleasant, but it happens. But I have already found two instances where Proton Bridge outright lies: 1) This issue. And, it can result in data loss. 2) When an email is deleted from All Mail folder, it pops back again, because All Mail contains also Thash (which is extremely non-standard and breaking). This causes complete schisma between the mail client and Proton Bridge to the point when the complete Bridge and mail client becomes unusable. I had to remove cache from Proton Bridge and account from the mail client (ThunderBird).

exander77 commented 4 years ago

ThunderBird has this feature using this plugin: https://addons.thunderbird.net/en-US/thunderbird/addon/copy-sent-to-current/

So you are probably breaking that as well.

And Apple Mail has it as well.

vrUVN

Not sure if they do different Message-IDs.

exander77 commented 4 years ago

It this fixed? I am afraid to test it.

skooda commented 4 years ago

Fixed by changing this behaviour in v1.2.8