Answers to messages contain debug code

[musicpanda]

The system sends messages like:
An answer to your message is available #ct124 #tcjua3kXPUZ3r0

The originating code is line 704 of controllers/admin/AdminCustomerThreadsController.php:
sprintf(Mail::l('An answer to your message is available #ct%1$s #tc%2$s', $ct->id_lang), $ct->id, $ct->token),

"Your message has been correctly sent" has the same problem.

[firstred]

The Customer Thread ID and Customer Thread token are added to the title, in order to recognize replies. This is by design for raw SMTP sends, since there is no way to recognize the thread ID otherwise.

[musicpanda]

I can't remember to have seen this solved this way by any other organization. It seems to me that there are other ways to solve the issue:

• for many shops having the mails sorted by email address would be enough. That alone might be a good enough argument to make this "feature" optional.
• one could put the identification data in the body of the mail - somewhere in the footer: just embed it in a recognizable start and end string. In that case one could even hide it (fontsize 1; text-color white). One could also at the logo to the mail

[musicpanda]

I checked some mail from Bax to see how they handle this:

• With orders they put numbers between brackets in the subject of the email. Like: "Your order has been shipped (1201024503)"
• for customer questions they ask the customer to provide a subject and they put a ticket number behind it. Like "How does my microphone work? [Ticket: 160730-000407]"

To me such solutions look a lot more elegant.

[firstred]

It certainly is a more elegant solution, but do note that it adds a security risk since email addresses can't be spoofed and due to the lack of a token.

Usually larger stores and services add an inbound domain which is connected to an incoming mail server that receives the replies and parses the messages that can be added to their messaging system. An example is the GitHub mail you just received. If you look at the Reply-To address you'll see that it accepts a token and some more info which adds a layer of authentication. That is how it's done securely. Unfortunately this is not possible with raw SMTP sends and the IMAP extension, so we have reserved this functionality for transactional email services that support this. (It's on the todo list for the mandrill module: https://github.com/thirtybees/mandrill)

[musicpanda]

Sorry, I fail to understand what you mean.

Why is adding "#ct124 #tcjua3kXPUZ3r0" to an email subject more secure than "[Ticket: 124-jua3kXPUZ3r0]"?

[Traumflug]

Why is adding "#ct124 #tcjua3kXPUZ3r0" to an email subject more secure than "[Ticket: 124-jua3kXPUZ3r0]"?

I think that's a valid point. People are much more used to [] stuff than to ##.

[Traumflug]

P.S.: @musicpanda, as you're apparently capable of writing code and look into the issue already, could you perhaps craft a patch? That's be awesome. Rumors say that @firstred is pretty covered in other areas of thirty bees code.

[musicpanda]

@Traumflug Maybe when I find time. Migrating one website has cost me already a lot more time than planned. Analyzing bugs close enough to be able to write a decent error report takes a lot of time. The consequence is that I am now far behind with the migration of another much bigger webshop - for which this was meant as a kind of tryout.

Anyway, I will only make a comment here on what I think should be changed. I don't do Github.

[Traumflug]

Feel welcome to send me patches to mah@jump-ing.de. For my taste, Github is way too complicated, too.

[musicpanda]

ok

[musicpanda]

Some further experiments:

I changed the code and added an escape of the filenames:
public static function addFiles(array $files) { foreach ($files as &$item) { $item = ['file' => Db::getInstance()->escape($item)]; } $success = true; foreach (array_chunk($files, static::CHUNK_SIZE) as $chunk) { $success &= Db::getInstance()->insert( static::TABLE, $files, false, false ); } return $success; }

This escaping works:

$item = ['file' => Db::getInstance()->escape($item)];

However, another problem appeared: the number of entries in the database table: 1893774. Nearly 2 million: everyone of the 13723 file names was 138 times present in the database. This is not the fault of the escape: when I ran another run with the original code I found 1899068. That is even a bit ( 5294) more.

Obviously this resulted in an endless backup and a huge backup file. I ended up switching off Apache in order to end the process.

[musicpanda]

I found a solution for this problem too.

The code "foreach (array_chunk($files, static::CHUNK_SIZE) as $chunk)" is at fault. With a chunk size of 100 this means that the 13724 files are divided in 138 chunks. Problem is however that each of those 138 times it is not sending 100 entries but the whole block of 13724.

However, even removing this loop didn't end the problems. When I now start the update and the backup has finished its 13724 entries it starts again. And again. And again. It seems an endless loop.

[Traumflug]

Wow, nice findings!

[musicpanda]

I fail to understand the code that causes the endless loop. What I see is that the function ajaxProcessBackupFiles() in classes/AjaxProcessor.php is called once again, stores again the filelist in the database, deletes the old backup and then starts filling a new one.

What is not clear to me is how and from where this function is called.

[Traumflug]

Magic at work.

PHP ajaxProcessBackupFiles() being called typically means there's some JavaScript doing a POST request with action: 'backupFile' in the request data section. This 'ajaxProcess' gets added automatically by the dispatcher somehow. I'd try looking into which requests get sent (visible in browser's developer toolbox), with which data and what the answers are. There has to be a stop sign somehow, the request being sent with zero files left has to be different from the previous ones.

And to not make this a tedious test of patience, I'd shorten the file list in PHP to just 100 files and chunk size to 20. Once the process works, this limitation can be reverted.

[musicpanda]

Wrong subject. I have put the comments back to thirtybees/tbupdater#9

[Traumflug]

This issue is solved, including its side aspects, isn't it?

[musicpanda]

Yes, let's close it