Re: [AppDb] Comments handling cleanup

[Tony Lambregts]

Jonathan Ernst wrote:
> Hi,
>
> I followed your discussions about sql/html injection and had a look at
> what you are doing in order to fix the situation and I don't want to
> offense anybody but I feel that what you are trying to do is a bit hacky
> ;-).
>
> Here is a cleaned up version of the comments handling that better fixes
> (imho) all problems related to sql and html injection.
>
> The code I made is not dependent on magic_quote value and should'nt
> allow anybody to mess with the values read or written from/to the
> database. Also it doesn't require to make copies of every single string the user
> passes to us.
>
> It is based on php manual's best practices for avoiding injection.
>
> I'd be very glad if we'll use such mechanism for the rest of the
> queries and get rid of compile_whatever, makeSafe & co.
>
> This patch follows the Coding Standards we set up a little bit more as
> well.
>
> Awaiting your comments.

I am more in favor of this approach than using makeSafe() and do the same thing 
as Chris's query_parameters() patch. However I am hard pressed to say whether 
this method is really better or safer than query_parameters().

I have had real issues with makeSafe() the primary one is that IMO the place to 
make sure that we are safe from sql injection is where we create the SQL. The 
makeSafe() did not do that.

The changes to that are only cosmetic and make it harder to see the actual 
changes. Formating changes should be in a separate patch.

Also I would really appreciate a "Files Changed:" section that lists the files 
changed/added/removed by this patch. I find that it really helps in reviewing 
patches.

Please resubmit with the formating changes in a separate patch.

--

Tony Lambregts

[AppDb] Comments handling cleanup

[Jonathan Ernst]

Le dimanche 25 juin 2006 à 10:59 -0600, Tony Lambregts a écrit :
[...]
> I am more in favor of this approach than using makeSafe() and do the same thing 
> as Chris's query_parameters() patch. However I am hard pressed to say whether 
> this method is really better or safer than query_parameters().
>
> I have had real issues with makeSafe() the primary one is that IMO the place to 
> make sure that we are safe from sql injection is where we create the SQL. The 
> makeSafe() did not do that.
>
> The changes to that are only cosmetic and make it harder to see the actual 
> changes. Formating changes should be in a separate patch.
>
> Also I would really appreciate a "Files Changed:" section that lists the files 
> changed/added/removed by this patch. I find that it really helps in reviewing 
> patches.
>
> Please resubmit with the formating changes in a separate patch.

Thanks for your comments.

I was aware that my changes weren't really atomic but I sent the patch
to get some comments.

If everyone agrees with the approach I'll make separate patches
tomorrow.

Thanks