Menu
▾
▴
Re: [WebERP-developers] New branch working for DB_query parameterisation work
|
From: Jon R <jr...@gm...> - 2014-03-28 08:12:58
|
sorry, gmail keeps switching to my work email *Sent from my iLaptop* On Fri, Mar 28, 2014 at 4:11 PM, Jonathan (Thành) <th...@ic...>wrote: > I finished ConnectDB_dbal but can't commit it. > > > On Fri, Mar 28, 2014 at 3:45 PM, Phil Daintree <ph...@lo...>wrote: > >> Yes please re perfect parameterized query function and maybe >> AccountGroups.php reworked >> I think twig undermines the readability. >> >> On 28 March 2014 8:33:35 PM NZDT, icedlava <ice...@gm...> wrote: >> >>> Hi Phil, >>> >>> but have you written your ideal new function - named >>>> other than DB_query - and where the parameter type is given explicitly >>>> >>>> >>>> rather than inferred? Would be interested to see that >>>> >>> >>> Did you want me to put it in the new branch? >>> >>> >>>> >>>> I am also rethinking the TableView class idea to eliminate the >>>> <tr><td></td></tr> all over the place - >>>> >>> >>> I would support that - I think we have too much dependence on tables for >>> >>> >>> layout generally in the themes and using divs would be better. >>> >>> I think we should think very carefully about what classes are used and >>> needed in the 'table less' themes and try and keep them to a >>> minimum. >>> CSS allows us to do great things with minimal markup. >>> >>> I do believe that tables are still important for data views which is >>> where their forte in use is. >>> >>> >>>> >>>> We are of course looking it almost a complete re-write here! >>>> >>> >>> Sounds like it lol! I'm up for it :) >>> >>> We could do with a list of other things to do while we are at it. >>> I guess TWIG is still out ? It maybe just me but I don't find the code >>> >>> >>> easy to look at as it's mixed thoroughly with html output. >>> I'd still like to separate it but understand this might be going too far >>> right now :p >>> >>> Cheers, >>> >>> >>> >>> On 28 Mar 2014, at 17:34, Phil Daintree wrote: >>> >>> >>> Jo, >>>> >>>> I can >>>> find it now - but have you written your ideal new function - >>>> named >>>> other than DB_query - and where the parameter type is given explicitly >>>> rather than inferred? Would be interested to see that... perhaps since >>>> >>>> >>>> we are doing this we should jump right on in? >>>> >>>> I am also rethinking the TableView class idea to eliminate the >>>> <tr><td></td></tr> all over the place - and perhaps maybe conversion >>>> to >>>> >>>> >>>> <div> with optional classes? that might more easily accommodate phones >>>> etc. We are of course looking it almost a complete re-write here! >>>> >>>> Let's go :-) >>>> >>>> Phil >>>> >>>> Phil Daintree >>>> Logic Works Ltd - +64 (0)275 567890 >>>> >>>> >>>> http://www.logicworks.co.nz >>>> >>>> On 28/03/14 19:07, icedlava wrote: >>>> >>>>> >>>>> Hi Phil, >>>>> >>>>> As you say, my revised DB_query() just escapes the parameters as >>>>>> they >>>>>> >>>>>> were previously - just that we have now moved the escaping to the >>>>>> place >>>>>> where it needs to happen rather than blanket escaping every >>>>>> $_POST/$_GET >>>>>> - so we are equally secure as we were ... unless I am wrong and >>>>>> >>>>>> >>>>>> those >>>>>> variables which should not be escaped are not escape >>>>>> >>>>> Yes, exactly - we are equally secure as we were before for database >>>>> transactions. >>>>> As you say, the whole reason that this topic came up was due to >>>>> >>>>> >>>>> non-contextual escaping of variables and the issues that has and is >>>>> continuing to cause in the code/database. >>>>> We have the opportunity now to fix it, but also improve the security >>>>> of >>>>> webERP as we fix the original variable problem, which is what I've >>>>> >>>>> >>>>> tried to do with the >>>>> version I committed. >>>>> >>>>> Thanks for your understanding! >>>>> >>>>> Cheers, >>>>> >>>>> >>>>> On 28 Mar 2014, at 16:27, Phil Daintree wrote: >>>>> >>>>> >>>>>> Hi Jo, >>>>>> >>>>>> As you say, my revised DB_query() just escapes the parameters as >>>>>> they >>>>>> were previously - just that we have now moved the escaping to the >>>>>> place >>>>>> where it needs to happen rather than blanket escaping every >>>>>> >>>>>> >>>>>> $_POST/$_GET >>>>>> - so we are equally secure as we were ... unless I am wrong and >>>>>> those >>>>>> variables which should not be escaped are not escaped. >>>>>> >>>>>> That said, I will look into the full prepare execute method in more >>>>>> >>>>>> >>>>>> detail. Of course with my blinkered vision what I did is more >>>>>> readable >>>>>> :-) I am persuaded by the argument to do things the most secure way >>>>>> though given we are into a major re-write and I think this may be >>>>>> the >>>>>> full prepare/execute method you first suggested. >>>>>> >>>>>> Phil >>>>>> >>>>>> Phil Daintree >>>>>> Logic Works Ltd - +64 (0)275 567890 >>>>>> >>>>>> >>>>>> http://www.logicworks.co.nz >>>>>> >>>>>> On 28/03/14 16:45, icedlava wrote: >>>>>> >>>>>>> >>>>>>> Hi Phil, >>>>>>> >>>>>>> For reference here on the list, I have committed the code as >>>>>>> requested. >>>>>>> You have updated the original function I committed which changes >>>>>>> and >>>>>>> I >>>>>>> did make a point to you in email that these changes will not work. >>>>>>> >>>>>>> >>>>>>> As per your request I've forwarded your response to the mailing >>>>>>> list >>>>>>> but >>>>>>> posting here for completeness in this thread. >>>>>>> >>>>>>> Note that the latest changes will not work because it is not the >>>>>>> fact >>>>>>> >>>>>>> >>>>>>> that the query is prepared, it is the fact the query is >>>>>>> paramaterized >>>>>>> that >>>>>>> makes it secure. >>>>>>> Parameterized queries, done in the way I did it work because the >>>>>>> query >>>>>>> string and the variables are sent separately to the database server >>>>>>> and >>>>>>> only there are put together. >>>>>>> >>>>>>> The changes you made are no different to the original DB_query that >>>>>>> >>>>>>> >>>>>>> mysql_db_escapes the data - except there is more code and separated >>>>>>> parameters. The key issue: >>>>>>> >>>>>>> In your new code: >>>>>>> $result=mysqli_query($Conn, $SQLString); is the problem. No >>>>>>> different >>>>>>> to >>>>>>> prior old DB_query. >>>>>>> >>>>>>> >>>>>>> The query is already 'put together' with query and variables before >>>>>>> it >>>>>>> goes to the server. That is the insecure nature of it and why I >>>>>>> changed >>>>>>> it. >>>>>>> >>>>>>> Appreciate your work and thought on it so far. >>>>>>> >>>>>>> >>>>>>> Cheers, >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 25 Mar 2014, at 17:10, Phil Daintree wrote: >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> I've made up a new branch so we can mess with this a bit >>>>>>>> >>>>>>>> svn checkout --username=XXXXX >>>>>>>> svn+ssh://XX...@sv.../p/web-erp/code/branches/working >>>>>>>> >>>>>>>> >>>>>>>> yourworkingcopy >>>>>>>> >>>>>>>> where XXXXX is your sourceforge login. >>>>>>>> >>>>>>>> Jo, if you commit the code you want for the new DB_query - and I >>>>>>>> propose >>>>>>>> we should keep it the same name - but with a new optional >>>>>>>> parameter >>>>>>>> >>>>>>>> >>>>>>>> for >>>>>>>> the array of parameters. >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Phil >>>>>>>> >>>>>>>> Phil Daintree >>>>>>>> Logic Works Ltd - +64 (0)275 567890 >>>>>>>> http://www.logicworks.co.nz >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> ------------------------------ >>>>>>>> >>>>>>>> Learn Graph Databases - Download FREE O'Reilly Book >>>>>>>> "Graph Databases" is the definitive new guide to graph databases >>>>>>>> and >>>>>>>> their >>>>>>>> applications. Written by three acclaimed leaders in the field, >>>>>>>> >>>>>>>> >>>>>>>> this first edition is now available. Download your free book >>>>>>>> today! >>>>>>>> http://p.sf.net/sfu/13534_NeoTech >>>>>>>> ------------------------------ >>>>>>>> >>>>>>>> Web-erp-developers mailing list >>>>>>>> >>>>>>>> >>>>>>>> Web...@li... >>>>>>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >>>>>>>> >>>>>>>> ------------------------------ >>>>>>> >>>>>>> ------------------------------ >>>>>>> >>>>>>> Web-erp-developers mailing list >>>>>>> Web...@li... >>>>>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >>>>>>> >>>>>>> >>>>>> >>>>>> ------------------------------ >>>>>> >>>>>> ------------------------------ >>>>>> >>>>>> Web-erp-developers mailing list >>>>>> Web...@li... >>>>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >>>>>> >>>>>> ------------------------------ >>>>> >>>>> ------------------------------ >>>>> >>>>> Web-erp-developers mailing list >>>>> Web...@li... >>>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >>>>> >>>> >>>> >>>> >>>> ------------------------------ >>>> >>>> ------------------------------ >>>> >>>> Web-erp-developers mailing list >>>> >>>> >>>> Web...@li... >>>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >>>> >>>> >>> ------------------------------ >>> >>> ------------------------------ >>> >>> Web-erp-developers mailing list >>> Web...@li... >>> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >>> >>> >> Phil Daintree >> +64(0)275 567890 >> skype: daintree >> >> >> ------------------------------------------------------------------------------ >> >> _______________________________________________ >> >> Web-erp-developers mailing list >> Web...@li... >> https://lists.sourceforge.net/lists/listinfo/web-erp-developers >> >> > |
