Information leakage in TggAtos::error

[emily-d]

class TggAtos 
...
public static function error
...
$errorlog = $error.(is_null($object) ? '' : PHP_EOL.'debug object: '.print_r($object));
Logger::addLog($errorlog);

If I'm not wrong, print_r without a second (true) parameter will leak the 'debug object' to the user screen and that's not something we want.

I tried with print_r($object, true) but this will generate a PrestaShop exception in the Logger validation method. This method (isMessage in Validate.php) requires that :

public static function isMessage($message)
{
    return !preg_match('/[<>{}]/i', $message);
}

So I ended up doing :

$debug_object = htmlentities(print_r($object,true),ENT_QUOTES,"UTF-8");
$errorlog = $error.(is_null($object) ? '' : PHP_EOL.'debug object: '. (Validate::isMessage($debug_object) ? $debug_object : 'Can\'t dump debug object to log.'));

I filter the debug object with htmlentities and I check that the debug object is valid before submitting it to the logger.

[TrogloGeek]

You're totally right, I must have been really tired writing this and forgetting to set the second parameter of print_r to true, I usually pay great attention on debugging methods, and thanks for so much details, it will save me alot of time. I wonder why they forbid those characters in messages, seems to me like an arbitrary decision, and the purpose of a logger is to be able to log anything, we should even be able to pass a object to log and it should format it for us (like the great Mage::log() in Magento). I'll correct the second parameter of print_r, strip forbidden characters: we'll replace them with HTML entities, in order not to loose to much informations (problem is that we won't be able in some cases to know if for exemple [stands for a literal [ or the literal [ itself), keep you check (in case they add more forbidden characters) and open an issue ticket in Prestashops GitHub.

[TrogloGeek]

I don't have time right now to run tests on this version, if some people have time to run some logging tests with this new version it would be awesome.

[TrogloGeek]

Prestashop issue tracker ticket http://forge.prestashop.com/browse/PSCFV-10591 please vote for it if you agree with it, or add you own opinion as comment

[emily-d]

Thanks for the quick fix. I'll try it and report if anything is wrong.

« I wonder why they forbid those characters in messages, seems to me like an arbitrary decision, and the purpose of a logger is to be able to log anything »

I guess that was the easy way to prevent XSS attacks in the Back Office by restricting the character set. But that's clearly not satisfactory.

[TrogloGeek]

No need to thank me for your own fix, result of your own diagnostic ;-) I just implemented it my way

[TrogloGeek]

Do some people have feedbacks about this version ? I still lack time to run the alpha-tests, being searching for a new job.

[TrogloGeek]

Oooops, huge mistake on this, I use $this->l() to translate error message in static context! Fixing it.