Serious bug leading to data loss

jalkut

Hi - thank you for the plugin - it mostly works great!

I ran into an issue where an important user was being deleted when it should not have been. Luckily I had backed up the database as I should have, so I was able to restore, but others might not be so careful.

The issue is in default.php's parsing of the delete list file. It doesn't take any care to avoid parsing the first line, which has the information about the file contents. Unfortunately this leads to the whole string being considered the first "part" and it tries to pass the whole string as a user ID to CleanserMode::DeleteAction. Because the value is not numeric, it skips over the parts of that function that clean out the user from Garden's own user model, but it does carry out the MySQL request to delete the user by ID.

In the end, an array like this is passed to Gdn::SQL()->Delete:

Array ( [UserID] => File Creation Time - December 4, 2014, 1:03 pm RoleID:3 (Offset: MR:5)pattern: )

And ultimately the SQL that is generated looks something like:

delete GDN_User from GDN_User where UserID = :UserID

And apparently if this runs it just deletes the first user it can find? I don't know but I ended up losing a valuable user. The behavior may be unpredictable but can definitely be damaging.

One way to work around this is simply to refuse to pass any user ID in to the DeleteAction unless it's in fact numeric:

`diff -r 383647952baf -r 226d6536945f default.php
--- a/default.php Thu Dec 04 13:25:45 2014 -0500
+++ b/default.php Thu Dec 04 13:26:38 2014 -0500

while(!feof($fp)) {
$infoline = fgets($fp);
$parts = explode('|', $infoline);

• if ($parts[0] > "2") {
• CleanserModel::DeleteAction($parts[0]);

•    $idToDelete = $parts[0];
  

•    if (is_numeric($idToDelete) && ($idToDelete > 2)) {
  

• CleanserModel::DeleteAction($idToDelete);
  }
  }
  fclose($fp);`

Hope this helps. Thanks again for the plugin,
Daniel

peregrine

And apparently if this runs it just deletes the first user it can find? I don't know but I ended up losing a valuable user. The behavior may be unpredictable but can definitely be damaging.

I couldn't replicate your problem. but thats a moot point now.
thanks for the feedback.

• I updated the plugin to 2.5 AND it will NOT delete id's that are not in the cleanser list.

• I added a better parse count.

• it will however delete userids listed in the cleanser list. (which it is supposed to do).

If you have a problem with cleanser version 2.5 deleting users not in the cleanserlist - please let me know.

peregrine

@jalkut said:
Hi - thank you for the plugin - it mostly works great!

I ran into an issue where an important user was being deleted when it should not have been. Luckily I had backed up the database as I should have, so I was able to restore, but others might not be so careful.

Yes, I am glad you followed the directions in the Readme and the Description. It is always a good idea to make backups of the database, especially when making mass deletions.
Alas, there will always be people who don't read the Readme, instructions, or the Description, and they will usually be the people who dig themselves into a hole.

you suggested not allowing deletion of userid 1 or userid 2

I added part of your idea of not allowing deletion of userid 1 even if the Admin wants to delete it via cleanser program or mistakenly includes it in the cleanser list.

and a couple of other changes that may help if a user decided to edit the cleanserlist manually and mangles the editing of the list.

as usual its worth upgrading your plugin to the latest version.