[Vtigercrm-developers] <RANT>When coding do not turn off warnings!</RANT>

[Alan Bell]

On 15/05/15 02:32, Hamono, Chris (DPC) wrote:
>
> Hi Alan
>
> Absolutely agree. But vtiger would not be producing “avalanches of 
> warnings” if these had been addressed earlier.
>
> My subject line says it all, *When Coding* do not turn off warnings.
>
> All I am trying to do is make sure people START using good coding 
> practices. It will take a long while for the code to catch up.
>
> With respect to patches…
>
> A major source of many of these warnings is the to_html() function, 
> weirdly it is called on every element returned by the DB. I assume 
> this was an early attempt at solving some sort of security issue.
>
it is also by some distance the most called function and the most time 
consuming bit of code. If you entirely trust every bit of data in the 
database you can just remove it (have it return the string unmodified) 
but that leaves you open to people putting stuff like <script> tags into 
fields and having them auto-execute when other people open the record. 
There are things you can do to make it cache results more efficiently 
and it would probably be better to do a major restructuring of the 
approach so that it runs a lot less and sanitises stuff later in the 
process.

This is what I did to make it faster
346,347d345
<
< global $htmlcache;//store the stripped HTML as we go along, a lot of 
the time we are processing the same strings
353d350
<         global $htmlcache;
358,360d354
<         if(isset($htmlcache[$string])){
<            return $htmlcache[$string];
<         }else{
387c381
<                 $clean = htmlentities($string, ENT_QUOTES, 
$default_charset);
---
 >                 $string = htmlentities($string, ENT_QUOTES, 
$default_charset);
389,392c383
<                 $clean = preg_replace(array('/</', '/>/', '/"/'), 
array('<', '>', '"'), $string);
<                         $htmlcache[$string]=$clean;
<                         return $clean;
<                   }
---
 >                 $string = preg_replace(array('/</', '/>/', '/"/'), 
array('<', '>', '"'), $string);

> Fixing it means removing it from the DB code because it is not a good 
> security solution and then dealing with the warnings it generates
>
> When I asked for why it is used I get no response. As such I don’t 
> know whether I should tackle it.
>

> My question about fixing the charts issue was met with “sure we will 
> consider your patch” even though it is a resolved problem.
>
> If the devs were more open with their open source perhaps these things 
> would be patched.
>
I have to say I do prefer the github workflow of pull requests rather 
than submitting patches by email.
>
> And yes that’s why I tried out Yetiforce I assume Blazej will gladly 
> accept patches. But it is nigh on impossible to switch gears this late 
> in the project. Perhaps the next project.
>
> Chris
>
> *From:*vtigercrm-developers-bounces at lists.vtigercrm.com 
> [mailto:vtigercrm-developers-bounces at lists.vtigercrm.com] *On Behalf 
> Of *Alan Bell
> *Sent:* Friday, 15 May 2015 6:28 AM
> *To:* vtigercrm-developers at lists.vtigercrm.com
> *Subject:* Re: [Vtigercrm-developers] <RANT>When coding do not turn 
> off warnings!</RANT>
>
> well there are development settings and production settings for a 
> reason, the idea is you develop with errors turned on, then turn them 
> off for production. It would be rather nice if vtiger wasn't such a 
> complete avalanche of warnings, it would make development easier. I 
> want to see errors I caused, much better than staring at a blank white 
> screen and guessing what the problem was! "Patches welcome" is a fair 
> response to this kind of thing, it isn't hard to address most 
> warnings, someone just has to get on and do it.
>
> Alan.
>
> On 14/05/15 21:55, Błażej Pabiszczak wrote:
>
>     I completely disagree with you. All good security practices, which
>     I have got familiar with, clearly describe principles for
>     displaying errors. A user should only see errors handled by the
>     application. Other errors such as sql, php, apache shouldn’t be
>     visible and I don’t think there are any arguments against it.
>
>     Not a single application is ideal, but displaying errors is a
>     serious breach of security and should never happen. A good example
>     are websites with web server errors [e.g. 403, 404] that should be
>     also handled by the application [should have its own error pages]
>     because hakers can get information about software and its version
>     from the default websites for server errors.
>
>     ---
>
>     Z poważaniem / Regards
>
>     *Błażej Pabiszczak*
>
>     /Chief Executive Officer/
>
>     M: +48.884999123
>     E: b.pabiszczak at yetiforce.com <mailto:b.pabiszczak at yetiforce.com>
>
>     W dniu 2015-05-14 03:02, Hamono, Chris (DPC) napisał(a):
>
>         A note to developers, vtiger, yetiforce or otherwise.
>
>         If you must recommend turning off php warnings in your code.
>         You are doing it wrong!
>
>         I cannot make this point strongly enough.
>
>         There is a reason all compilers and interpreters spit out
>         massive amounts of warnings. It’s because these warnings
>         indicate where your code is SLOPPY.
>
>         By ignoring those warnings you are potentially coding security
>         risks and buggy code. uninitialized variables are the most
>         common source of warnings and also the most common source of bugs.
>
>         So if you tell users they must turn off warnings it’s a sign
>         that the code is poorly written.
>
>         Chris
>
>         _______________________________________________
>         http://www.vtiger.com/
>
>
>
>
>     _______________________________________________
>
>     http://www.vtiger.com/
>
>
>
> _______________________________________________
> http://www.vtiger.com/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.vtigercrm.com/pipermail/vtigercrm-developers/attachments/20150515/ae979fe1/attachment-0001.html>