[AppDB] - make variables more consistent, fix indenting

[Tony Lambregts]

There are two issues here

1. large patches/commits are difficult to debug, test and fix.

2. Escaping input does not make the queries safe. We need to implement 
security by making the queries themselves safe.

See below:

Chris Morgan wrote:
> On Tuesday 20 June 2006 5:03 pm, Tony Lambregts wrote:
>   
>> Chris Morgan wrote:
>>     
>>> On Tuesday 20 June 2006 4:12 pm, Tony Lambregts wrote:
>>>       
>>>> Chris Morgan wrote:
>>>>         
>>>>> Make the code more consistent by using the appdb coding standards.
>>>>>
>>>>> Change $result = query_appdb(...) to $hResult = ... etc.
>>>>>
>>>>> Also fix some odd indenting due to spaces vs. tabs.
>>>>>
>>>>> There are still a ton of variables that are integers or strings that
>>>>> should be prefixed with 'i' or 's' if anyone is interested in cleaning
>>>>> some of that up.
>>>>>
>>>>> Chris
>>>>>           
>>>> I am not opposed to doing this but I think that the patch is way too big.
>>>> At the momment we still have issues with the last "big patch" that are
>>>> not cleared up [1]. On the whole unless we cannot avoid it I really do
>>>> not think that we should be making such large changes in one go like
>>>> this. Please break up the patch into smaller patches. That way it is
>>>> easier to find and fix any issues that arise.
>>>>
>>>> [1] New versions end up as orphans with current setup.
>>>>
>>>> --
>>>>
>>>> Tony Lambregts
>>>>         
>>> Would per-directory patches be easier to read?  Like one for /include,
>>> one for /admin, one for / ?
>>>
>>> This patch is a precursor to a patch that will close up a bunch of
>>> security holes that could result in the db being destroyed, then on to
>>> finding a more appropriate solution to the makeSafe() patch.
>>>
>>> Chris
>>>       
>> No That is not what I would prefer I would prefer a single patch for each
>> file that is changed (ie one for addcomment.php and one for
>> include/vendor.php. If the changes to one file need changes in other in
>> order for the appdb to continue to function then they should be together in
>> one patch.
>>
>>     
>
> There is no way I'm going to send a patch in per-file.  Its a huge pain in the 
> ass and doesn't change how the monolithic patch is reviewed since you'll end 
> up reviewing the same code anyway.  Breaking the patch up given that its a 
> atomic noop change is also not necessary.
>   
It may be a pain in the ass for you but it is an even bigger pain in the 
ass for everyone else when things break. With one big patch I cannot do 
a  "CVS update -D" to find exactly where things broke or just back out 
the broken part easily. Also I cannot easily test a monolithic patch but 
I  apply and test small patches. My compromise would be to have no more 
than 4 files changed in a patch unless it cannot be avoided.
>> Now you may think that is overboard but I have seen these "big patches"
>> break the AppDB too many times.
>>
>> As far as "security holes that could result in the db being destroyed" we
>> do have backup. I dont want you to think I do not care about security. That
>> is simply not true. I am however very much against breaking the AppDB for
>> regular use. The fact is that so far we have lost more data through bad
>> patches then through security holes.
>>
>>     
>
> I agree.  I'll fix the existing issues before applying any more patches like 
> this one.
>   
Right now makesafe() does nothing because you turned it off.  but with 
it turned on it breaks things and there are still real security issues 
to address. I'm not sure what else is broken at the moment but I know 
that there is some problem  administering the user account's where you 
do not see the users data.
>   
>> We have preaty good security in place already, We check that id's are
>> numeric and escape date before running it. Right now if you ask me we would
>> be better off making a audit of our querys than what you are advocating.
>>
>>     
>
> Our security is awful.  You should really re-read the email sent to the appdb 
> list by Mortiz Naumann and see how bad it is.
>
>   
This is exactly why I think we should audit queries.  The "makesafe" 
patch does nothing to address the issues he brings up. If the queries 
are not safe then we need to fix them.  See the patch I sent to you and 
appdb at winehq.org

--

Tony Lambregts