Re: [AppDB] - make variables more consistent, fix indenting
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 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
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. 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. 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. -- Tony Lambregts.
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.
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.
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. Chris
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(a)winehq.org -- Tony Lambregts
participants (2)
-
Chris Morgan -
Tony Lambregts