Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6475 closed Bug/Something is broken (fixed)

deleting on roundcube

Reported by: https://id.mayfirst.org/dkg Owned by: https://id.mayfirst.org/dkg
Priority: High Component: Tech
Keywords: roundcube courier imap Cc:
Sensitive: no

Description

#6357, #6462, and #6361 describe scenarios where the user was unable to delete messages in roundcube because of a missing .Trash folder.

If a .Trash folder is required to be able to delete messages, roundcube needs to create that folder if it is absent.

Change History (15)

comment:1 Changed 5 years ago by https://id.mayfirst.org/ross

  • Owner set to https://id.mayfirst.org/dkg
  • Status changed from new to assigned

comment:2 Changed 5 years ago by https://id.mayfirst.org/dkg

  • Keywords courier imap added
  • Priority changed from Medium to Low

So, if i move my ~/Maildir/.Trash out of the way or destroy it manually (e.g. rm -rf ~/Maildir/.Trash), and then try to delete a message in roundcube, the maildir appears to get recreated automatically, and there is no error.

If i remove the .Trash folder and immediately mkdir ~/Maildir/.Trash.foo, and then try to delete a message, then roundcube shows me "Connection to storage server failed."

I suspect this is because courier presents .Trash as an IMAP folder that contains a sub-folder named "foo" -- so the IMAP folder "Trash" is present, but because there is no maildir for .Trash itself, there is no way for a message to be stored in it.

I don't know whether this should be seen as a bug in courier, in our IMAP proxy daemon, in roundcube's assumptions about how to behave with an IMAP daemon, in the IMAP spec, or in me just doing something One Must Not Do™ (e.g. making a subfolder without the parent) with courier's use of maildir.

I'm reducing priority because i haven't seen this problem directly in the real world, though, and i haven't sorted out a way to replicate it through a process that a normal user might do. If someone wants to take this on and figure out how to replicate the problem in a real-world way, that'd be great!

I note that this has been discussed upstream, and they appear to think it's been handled (at least the discussion there mentions needing a Trash folder for deletion, even though that appears to have been slightly lost track of by the resolution.

comment:3 Changed 5 years ago by https://id.mayfirst.org/dkg

See comment:ticket:6462:12 -- it's possible that there is some interplay with courier's notion of subscribed folders.

comment:4 Changed 5 years ago by https://id.mayfirst.org/dkg

  • Priority changed from Low to Urgent

With #6516, it sounds like many people are having this problem. This needs to be resolved.

comment:5 Changed 5 years ago by https://id.mayfirst.org/dkg

comment:6 Changed 5 years ago by https://id.mayfirst.org/dkg

fwiw, the only way i could get this to reliably happen during my testing was to manually remove ~/Maildir/.Trash from the user's account.

ross says that one of his users is now permanently deleting messages even though there is no trash folder.

Last edited 5 years ago by https://id.mayfirst.org/dkg (previous) (diff)

comment:7 Changed 5 years ago by https://id.mayfirst.org/ross

This is correct. I have no Maildir/.Trash directory, but if I delete a message, the deletion is reported as successful and the message is gone. However, it is permanently deleted as there is no directory for it to be moved to.

If I change preferences, to use the Drafts directory as the Trash directory, then deleted messages are placed in the Drafts directory. In this case, though, the Drafts icon becomes the Trash icon in the UI. In no context have I been able to have a Maildir/.Trash directory automatically created by roundcube.

comment:8 Changed 5 years ago by https://id.mayfirst.org/dkg

Ross, are you able to create a directory directly via IMAP?

connect with:

gnutls-cli --port imaps mail.mayfirst.org

Then, after the IMAP connection happens, try the following three commands:

A LOGIN {USER} {PASS}
B CREATE INBOX.Trash
C LOGOUT

Here's an example session, with the server's responses included:

0 dkg@pip:~$ gnutls-cli --port imaps mail.mayfirst.org
Processed 94 CA certificate(s).
Resolving 'mail.mayfirst.org'...
Connecting to '209.234.253.240:993'...
- Peer's certificate is trusted
- The hostname in the certificate matches 'mail.mayfirst.org'.
- Successfully sent 0 certificate(s) to server.
- Session ID: C0:22:D8:39:45:F1:2A:69:6F:57:9A:76:67:3B:14:2B:2D:CF:96:80:DF:E6:1C:CC:17:99:1C:15:31:9A:FF:82
- Server has requested a certificate.
- Certificate type: X.509
- Got a certificate list of 2 certificates.
- Certificate[0] info:
 - subject `serialNumber=ajzC1Rsev2DCp9Nfp-BxSftoJu/SBl8C,C=US,O=mail.mayfirst.org,OU=GT84944489,OU=See www.rapidssl.com/resources/cps (c)11,OU=Domain Control Validated - RapidSSL(R),CN=mail.mayfirst.org', issuer `C=US,O=GeoTrust\, Inc.,CN=RapidSSL CA', RSA key 2048 bits, signed using RSA-SHA1, activated `2011-06-09 00:36:18 UTC', expires `2013-06-11 17:46:08 UTC', SHA-1 fingerprint `76ae64b98d96b23f9e3bba11907ab79e54f996ef'
	Public Key Id:
		81465e58b8c7a1799fda1b410af1cfe63cee40d8
	Public key's random art:
		+--[ RSA 2048]----+
		|      o+o        |
		|     oo=.        |
		|      ==o..      |
		|     .+++*       |
		|      .oE.=.     |
		|       . +o.     |
		|        .o=      |
		|        .o.o     |
		|         .+.     |
		+-----------------+

- Certificate[1] info:
 - subject `C=US,O=GeoTrust\, Inc.,CN=RapidSSL CA', issuer `C=US,O=GeoTrust Inc.,CN=GeoTrust Global CA', RSA key 2048 bits, signed using RSA-SHA1, activated `2010-02-19 22:45:05 UTC', expires `2020-02-18 22:45:05 UTC', SHA-1 fingerprint `c039a3269ee4b8e82d00c53fa797b5a19e836f47'
- Version: TLS1.0
- Key Exchange: RSA
- Cipher: AES-128-CBC
- MAC: SHA1
- Compression: NULL
- Handshake was completed

- Simple Client Mode:

* OK [CAPABILITY IMAP4rev1 UIDPLUS CHILDREN NAMESPACE THREAD=ORDEREDSUBJECT THREAD=REFERENCES SORT QUOTA IDLE ACL ACL2=UNION LOGINDISABLED] perdition ready on paulo.mayfirst.org 0002a9c0
A LOGIN dkgdkg notmyrealpassword
A OK You are so in
B CREATE INBOX.Bananas
B OK "INBOX.Bananas" created.
C CREATE INBOX.Trash
C NO Cannot create this folder.
D LOGOUT
* BYE Courier-IMAP server shutting down
D OK LOGOUT completed
*** Fatal error: The TLS connection was non-properly terminated.
*** Server has terminated the connection abnormally.
1 dkg@pip:~$ 

If you want to experiment further, you might want to read up on the other IMAP protocol options.

comment:9 Changed 5 years ago by https://id.mayfirst.org/ross

Yep, I am able to create the directory directly via IMAP and gnutls cli. Roundcube recognized it, but did not auto-subscribe me to the .Trash directory. And when I did subscribe to the directory, roundcube still did not put deleted emails into the directory until I specified 'Trash' as the configured folder for Trash in the special folders preferences.

comment:10 Changed 5 years ago by https://id.mayfirst.org/dkg

  • Priority changed from Urgent to High

ross, i'm unable to reliably replicate this. Can you describe the state of your mailboxes and the state of your roundcube preferences that leave you in the undeletable state?

I'm reducing the priority because i think having made the following change to stallman:/srv/roundcube/config/main.inc.php most of our users should now be OK:

$rcmail_config['create_default_folders'] = true;

comment:11 Changed 5 years ago by https://id.mayfirst.org/joseph

After upgrading roundcube (#6488), we applied patch in comment:5 to 0.8.4.

Checked diff on roundcube-dev

0 roundcube-code@stallman:/srv/roundcube-dev$ git diff
diff --git a/program/steps/mail/move_del.inc b/program/steps/mail/move_del.inc
index a9e4bd8..44f1ca9 100644
--- a/program/steps/mail/move_del.inc
+++ b/program/steps/mail/move_del.inc
@@ -37,6 +37,9 @@ if ($RCMAIL->action=='moveto' && !empty($_POST['_uid']) && strlen($_POST['_targe
     $target = get_input_value('_target_mbox', RCUBE_INPUT_POST, true);
     $mbox = get_input_value('_mbox', RCUBE_INPUT_POST, true);

+    if (!$RCMAIL->storage->folder_exists($target)) {
+      $RCMAIL->storage->create_folder($target, true);
+    }
     $moved = $RCMAIL->storage->move_message($uids, $target, $mbox);

     if (!$moved) {

Tested appling diff on roundcube-dev to roundcube

0 roundcube-code@stallman:/srv/roundcube-dev$ git diff | ( cd ../roundcube &&  patch --dry-run -p1 )
patching file program/steps/mail/move_del.inc

Applied diff on roundcube

0 roundcube-code@stallman:/srv/roundcube-dev$ git diff | ( cd ../roundcube &&  patch -p1 )             
patching file program/steps/mail/move_del.inc

Checked diff on roundcube

0 roundcube-code@stallman:/srv/roundcube-dev$ (cd ../roundcube && git diff)                            
diff --git a/program/steps/mail/move_del.inc b/program/steps/mail/move_del.inc
index a9e4bd8..44f1ca9 100644
--- a/program/steps/mail/move_del.inc
+++ b/program/steps/mail/move_del.inc
@@ -37,6 +37,9 @@ if ($RCMAIL->action=='moveto' && !empty($_POST['_uid']) && strlen($_POST['_targe
     $target = get_input_value('_target_mbox', RCUBE_INPUT_POST, true);
     $mbox = get_input_value('_mbox', RCUBE_INPUT_POST, true);
 
+    if (!$RCMAIL->storage->folder_exists($target)) {
+      $RCMAIL->storage->create_folder($target, true);
+    }
     $moved = $RCMAIL->storage->move_message($uids, $target, $mbox);
 
     if (!$moved) {
Last edited 5 years ago by https://id.mayfirst.org/joseph (previous) (diff)

comment:12 Changed 5 years ago by https://id.mayfirst.org/dkg

I've just set up an mfpl roundcube git repo for us to publicly track our work.

You can:

git clone git://git.mayfirst.org/mfpl/roundcube

to see it.

Last edited 5 years ago by https://id.mayfirst.org/dkg (previous) (diff)

comment:13 Changed 5 years ago by https://id.mayfirst.org/joseph

  • Resolution set to fixed
  • Status changed from assigned to closed

We've committed this change to the git repo in comment:12. We'll deal with the divergences with upstream as we update to future versions, where hopefully this change will be incorporated.

comment:14 Changed 5 years ago by https://id.mayfirst.org/dkg

upstream responded to my patch submission, suggesting it was redundant with other code actively in use (see public function move_message() in program/include/rcube_imap.php). And indeed, i just tried reverting our patch on roundcube.dev.mayfirst.org, and i don't seem to see any problem with deleting messages.

So i still don't understand why this happened, or failed so regularly for other users (other than ross's suggestion about a mis-created maildir). But i'm rebasing the mfpl branch for roundcube to drop that patch.

comment:15 Changed 5 years ago by https://id.mayfirst.org/dkg

Updating to the new version of the branch on the roundcube site looks like:

0 roundcube-code@stallman:/srv/roundcube$ git checkout mfpl/mfpl && git branch -D mfpl && git checkout -b mfpl mfpl/mfpl
Note: checking out 'mfpl/mfpl'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at c382143... (fetch_identity_objects): avoid redundant call to unserialize
Deleted branch mfpl (was 9d2eb3e).
Branch mfpl set up to track remote branch mfpl from mfpl.
Switched to a new branch 'mfpl'
0 roundcube-code@stallman:/srv/roundcube$ 

Please login to add comments to this ticket.

Note: See TracTickets for help on using tickets.