Code review comment for lp:~camptocamp/openobject-addons/7.0-fix_1196847

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

I think disabling the cron altogether is a wrong move. The level of criticality of this alleged vulnerability is quite low, while the mechanism to send system messages to OpenERP administrators is important and should not need to be manually turned on. It was used in the past to notify all users (not just OE/OPW customers!) about a critical vulnerability in OpenERP 6.0 that needed to be patched urgently [1], and it was levels of magnitude more serious than the potential exploit of this notification mechanism.

It seems to me that replacing safe_eval() with ast.literal_eval() and forcing an HTTPS URL would reduce the possibilities of misuse to an acceptable level. The handling of the return values is the actual mechanism used to notify system administrators of important messages.

BTW if you think safe_eval is not safe you should post another bug report with the issues you've found with it, so that we can fix them - as this would have a much bigger impact!

Also, log messages are not be translated, and this particular extra logging seems a bit superfluous. The mechanism is meant to be fault-tolerant on purpose.

[1] bug 832601

review: Needs Fixing

« Back to merge proposal