Menu
▾
▴
web-erp-developers
|
From: Phil D. <ph...@lo...> - 2014-03-25 06:40:57
|
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 |
|
From: icedlava <ice...@gm...> - 2014-03-25 07:26:00
|
> I've made up a new branch so we can mess with this a bit Thanks for setting that up Phil. > we should keep it the same name - but with a new optional parameter > for > the array of parameters. Ok, so, in order to keep consistency with the existing function signature, we should likely have the new $bindvar array of parameters at the end of the signature list. I'll start with that and we can change things as we need. 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 |
|
From: icedlava <ice...@gm...> - 2014-03-28 03:45:39
|
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 |
|
From: Phil D. <ph...@lo...> - 2014-03-28 05:57:22
|
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 > |
|
From: icedlava <ice...@gm...> - 2014-03-28 06:08:03
|
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 |
|
From: Phil D. <ph...@lo...> - 2014-03-28 07:05:11
|
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 > |
|
From: icedlava <ice...@gm...> - 2014-03-28 07:34:00
|
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 |
|
From: Jon R <jr...@gm...> - 2014-03-28 07:38:09
|
If we use the Views classes TWIG can be implemented in a theme. I will post a proof of concept to the github site in the next day or so. ~serakfalcon *Sent from my iLaptop* On Fri, Mar 28, 2014 at 3:33 PM, 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 > |
|
From: Phil D. <ph...@lo...> - 2014-03-29 22:32:56
|
Nice ... Just one thought ... do we need to have the include/Doctrine/lib directories couldn't we just go include/Doctrine/Common and include/Doctrine/DBAL folders directly as the includes/Doctrine directory is empty and the includes/Doctrine/lib is empty other than the Doctrine folder just want to be able to get at the meat :-) Phil Phil Daintree Logic Works Ltd - +64 (0)275 567890 http://www.logicworks.co.nz On 28/03/14 20:38, Jon R wrote: > If we use the Views classes TWIG can be implemented in a theme. I will > post a proof of concept to the github site in the next day or so. > ~serakfalcon > > /Sent from my iLaptop/ > > > On Fri, Mar 28, 2014 at 3:33 PM, icedlava <ice...@gm... > <mailto: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 > <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... > https://lists.sourceforge.net/lists/listinfo/web-erp-developers |
|
From: Jon R <jr...@gm...> - 2014-03-30 01:38:29
|
Yes we could, the only reason it is structured like that is to make updating Doctrine a simple copy-paste but it's not necessary On Mar 30, 2014 6:33 AM, "Phil Daintree" <ph...@lo...> wrote: > Nice ... > > Just one thought ... do we need to have the > > > include/Doctrine/lib directories > > couldn't we just go > > include/Doctrine/Common > and > include/Doctrine/DBAL > > folders directly as the includes/Doctrine directory is empty and the > includes/Doctrine/lib is empty other than the Doctrine folder just want to > be able to get at the meat :-) > > Phil > > Phil Daintree > Logic Works Ltd - +64 (0)275 567890http://www.logicworks.co.nz > > On 28/03/14 20:38, Jon R wrote: > > If we use the Views classes TWIG can be implemented in a theme. I will > post a proof of concept to the github site in the next day or so. > ~serakfalcon > > *Sent from my iLaptop* > > > On Fri, Mar 28, 2014 at 3:33 PM, 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 >> > > > > ------------------------------------------------------------------------------ > > > > _______________________________________________ > Web-erp-developers mailing lis...@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 > > |
|
From: Phil D. <ph...@lo...> - 2014-03-30 02:04:23
|
Probably not worth changing then... Phil Phil Daintree Logic Works Ltd - +64 (0)275 567890 http://www.logicworks.co.nz On 30/03/14 14:38, Jon R wrote: > > Yes we could, the only reason it is structured like that is to make > updating Doctrine a simple copy-paste but it's not necessary > > On Mar 30, 2014 6:33 AM, "Phil Daintree" <ph...@lo... > <mailto:ph...@lo...>> wrote: > > Nice ... > > Just one thought ... do we need to have the > > > include/Doctrine/lib directories > > couldn't we just go > > include/Doctrine/Common > and > include/Doctrine/DBAL > > folders directly as the includes/Doctrine directory is empty and > the includes/Doctrine/lib is empty other than the Doctrine folder > just want to be able to get at the meat :-) > > 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:38, Jon R wrote: >> If we use the Views classes TWIG can be implemented in a theme. I >> will post a proof of concept to the github site in the next day >> or so. >> ~serakfalcon >> >> /Sent from my iLaptop/ >> >> >> On Fri, Mar 28, 2014 at 3:33 PM, icedlava <ice...@gm... >> <mailto: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 >> <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... > <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 |
|
From: icedlava <ice...@gm...> - 2014-03-28 07:43:06
|
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 |
|
From: Phil D. <ph...@lo...> - 2014-03-28 08:33:46
|
OK why not... I have seen it before I think but would be interested in other views on this. 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 > |
|
From: Phil D. <ph...@lo...> - 2014-03-29 22:28:31
|
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 > |
|
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 > |
|
From: Jon R <jr...@gm...> - 2014-03-31 02:28:55
|
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...> 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...> 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 >> > |
|
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 |
|
From: Phil D. <ph...@lo...> - 2014-03-28 07:46:04
|
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 |
|
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 > > |
|
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 >> >> > |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X