Menu
▾
▴
Re: [WebERP-developers] New branch working for DB_query parameterisation work
|
From: Jon R <jr...@gm...> - 2014-03-30 03:41:55
|
Hi Phil, the main issue is icedlava's code returns an array and all the support functions expect a mysqli_result object. If you have: a) DB_query returns an array also, and the support functions expect an array (hard to implement for the fetch functions) b) we take the array result into account and handle DB_Preparedquery differently from DB_Query (i don't recommend this) or, Hi Phil, the main issue is icedlava's code returns an array and all the support functions expect a mysqli_result object. If you have: a) DB_query returns an array also, and the support functions expect an array (hard to implement for the fetch functions) b) we take the array result into account and handle DB_Preparedquery differently from DB_Query (i don't recommend this) or, Hi Phil, the main issue is icedlava's code returns an array and all the support functions expect a mysqli_result object. If you have: a) DB_query returns an array also, and the support functions expect an array (hard to implement for the fetch functions) b) we take the array result into account and handle DB_Preparedquery differently from DB_Query (i don't recommend this) or, Hi Phil, the main issue is icedlava's code returns an array and all the support functions expect a mysqli_result object. If you have: a) DB_query returns an array also, and the support functions expect an array (hard to implement for the fetch functions) b) we take the array result into account and handle DB_Preparedquery differently from DB_Query (i don't recommend this) or, c) we return the mysqli_stmt back, which has issues with binding the return values, and that DB_Query returns a mysqli_result object, which handles differently from mysqli_stmt On Mar 30, 2014 6:29 AM, "Phil Daintree" <ph...@lo...> wrote: > I had a go at removing the $Conn parameter and renaming the function > > DB_PreparedQuery() > > leaving the DB_query() function where it is - no reason why we can't > continue to use it where there are no parameters involved I guess - as > you suggested Jo... > > I messed with the style to try to bring the new code into line with the > conventions we use and make the variable names a bit more meaningful - I > hate abbreviated variable names that give you no clue as to what is > going on... it is obvious when you write the code but 3 or 4 sleeps > later not so obvious ... curly braces on the same line as the function > call/if test etc etc. > > and of course predictably I have messed it up - login and > AccounGroups.php no longer work :-( > > Hopefully though the concept is clear. > > Phil > > Phil Daintree > Logic Works Ltd - +64 (0)275 567890 > http://www.logicworks.co.nz > > On 28/03/14 20:42, icedlava wrote: > > Phil, > > > > Can I commit TWIG to the new branch and a simple integration - existing > > code will work as normal but it would provide somewhere to add twig > > templates and themes for proof of concept or comparison? > > > > If it is not required, or wanted, it could be removed later. > > > > What do you think? > > > > > > > > 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 > > > > > > ------------------------------------------------------------------------------ > _______________________________________________ > Web-erp-developers mailing list > Web...@li... > https://lists.sourceforge.net/lists/listinfo/web-erp-developers > |
