Menu
▾
▴
Re: [WebERP-developers] New branch working for DB_query parameterisation work
|
From: Phil D. <ph...@lo...> - 2014-03-31 08:58:25
|
Crickey, seems pretty tough to get any output from prepare/execute. This feels very messy to me... nothing pretty about it! Phil Phil Daintree Logic Works Ltd - +64 (0)275 567890 http://www.logicworks.co.nz On 31/03/14 15:28, Jon R wrote: > Sorry about the repeating there, was on my phone and it does weird things. > I figured out a work around for the DB_PreparedQuery issues, I made a > class that has most of the functions of mysqli_result, and > DB_PreparedQuery passes that back. That way, the DB functions can > handle it as if it was a mysqli_result object. I've updated the > working branch accordingly. > I also moved $Parameters to the second position, so you don't have to > specify values for everything (since DB_Query is now different > $Parameters doesn't need to be last anymore), and reduced reliance on > passing $db around, although legacy code will still work. I recommend > removing $db from code outside of the connection includes wherever > possible. > > /Sent from my iLaptop/ > > > On Sun, Mar 30, 2014 at 11:41 AM, Jon R <jr...@gm... > <mailto:jr...@gm...>> wrote: > > 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... > <mailto: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 > <tel:%2B64%20%280%29275%20567890> > 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 > <tel:%2B64%20%280%29275%20567890> > >> 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 > <tel:%2B64%20%280%29275%20567890> > >>>> 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 <http://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 > <tel:%2B64%20%280%29275%20567890> > >>>>>> 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... > <mailto:Web...@li...> > >>>>>> > https://lists.sourceforge.net/lists/listinfo/web-erp-developers > >>>>> > ------------------------------------------------------------------------------ > >>>>> _______________________________________________ > >>>>> Web-erp-developers mailing list > >>>>> Web...@li... > <mailto:Web...@li...> > >>>>> > https://lists.sourceforge.net/lists/listinfo/web-erp-developers > >>>>> > >>>> > ------------------------------------------------------------------------------ > >>>> _______________________________________________ > >>>> Web-erp-developers mailing list > >>>> Web...@li... > <mailto:Web...@li...> > >>>> > https://lists.sourceforge.net/lists/listinfo/web-erp-developers > >>> > ------------------------------------------------------------------------------ > >>> _______________________________________________ > >>> Web-erp-developers mailing list > >>> Web...@li... > <mailto:Web...@li...> > >>> > https://lists.sourceforge.net/lists/listinfo/web-erp-developers > >>> > >> > >> > ------------------------------------------------------------------------------ > >> _______________________________________________ > >> Web-erp-developers mailing list > >> Web...@li... > <mailto:Web...@li...> > >> https://lists.sourceforge.net/lists/listinfo/web-erp-developers > > > ------------------------------------------------------------------------------ > > _______________________________________________ > > Web-erp-developers mailing list > > Web...@li... > <mailto:Web...@li...> > > https://lists.sourceforge.net/lists/listinfo/web-erp-developers > > > > > ------------------------------------------------------------------------------ > _______________________________________________ > Web-erp-developers mailing list > Web...@li... > <mailto: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 |
