Menu
▾
▴
Re: [WebERP-developers] New branch working for DB_query parameterisation work
|
From: Jonathan (T. <th...@ic...> - 2014-03-28 08:12:05
|
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 > > |
