Post Reply 
 
Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
DB_escape_string
03-14-2014, 09:02 PM
Post: #31
RE: DB_escape_string
(03-14-2014 08:33 PM)icedlava Wrote:  
(03-11-2014 05:03 AM)Forums Wrote:  Would it be best to change the relevant ConnectDB file to just use the correct escape_string() function. That way whichever driver the user is using they will get the correct function?

Thanks
Tim

Yes, that is something that has to be done, in conjunction with other fixes. Doing it now will ensure entities are not going into the database which is good. But there will be code around that assumes entities are already taken care of when displaying to the browser, so will in fact not be once this is changed unless each is treated individually.

Hi Jo, can you give me an example so I can look into it?

thanks
Tim
Quote this message in a reply
03-16-2014, 03:19 AM
Post: #32
RE: DB_escape_string
I seem to remember the htmlspecialchars() call in the DB_escape_string() function is a relatively new addition anyway.

Tim
Quote this message in a reply
03-20-2014, 08:31 PM (This post was last modified: 03-20-2014 08:40 PM by serakfalcon.)
Post: #33
RE: DB_escape_string
OK I have made a function that will provide this functionality. It can be used alongside the existing code, so while we should update everything to use the new code, all the existing code will still work in the meantime.
It requires a minor DB change:
Code:
ALTER TABLE `audittrail`
CHANGE COLUMN `transactiondate` `transactiondate` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ,
ADD COLUMN `params` TEXT NULL AFTER `querystring`;

If we're adding a column I figured we might as well use TIMESTAMP, since it will accept datetimes (so the old code will work) but new code will not have to pass a date value, let SQL do the work.
(On another note, pretty much every DB has a way to log every SQL transaction, so the DB itself could do logging)

Other changes: since $db is the global connection variable, it isn't passed to the function instead the function uses the global $db.
Also, since the prepared statement must be closed inside the function, the function returns an array of rows, with each row being an associative array with the keys being the column name. So for example with the SQL
Code:
SELECT transactiondate, userid, querystring
FROM audittrail LIMIT 5
if we took $result[0]['userid'] we'd get the userid of the first result from this query.

Code:
function DB_bindquery($query,
                    $bindvars = null,
                    $ErrorMessage='',
                    $DebugMessage= null,
                    $Transaction=false,
                    $TrapErrors=true) {

    global $db;
    global $debug;
    global $PathPref;
    
    if (!isset($DebugMessage)) {
        $DebugMessage = _('The SQL that failed was');
    }
    
        //sanitize the string
    $query = filter_var($query, FILTER_SANITIZE_STRING);
        // prepare for bound query
    if (!($stmt = $db->prepare($query)) && $TrapErrors) {
        require_once($PathPrefix . 'includes/header.inc');
        prnMsg($ErrorMessage . '<br />' . $db->error,'error', _('Syntax Error'). ' ' .$db->errno);
        if ($debug==1){
            prnMsg($DebugMessage. '<br />' . $query . '<br />','error',_('Prepare Statement Failed'));
        }
        include($PathPrefix . 'includes/footer.inc');
        exit;
        
    }
    
    //prepare variables if variables are passed
    if (is_array($bindvars) === true) {
        $params = array(''); // Create the empty 0 index
        foreach ($bindvars as $val) {

            switch (gettype($val)) {
                case 'NULL':
                case 'string':
                    $params[0] .= 's';
                    break;

                case 'integer':
                case 'boolean':
                    $params[0] .= 'i';
                    break;
                    
                case 'object':
                case 'array':
                    //are these ever saved into the db?
                    $params[0] .= 'b';
                    $val = json_encode($val);
                    break;
                    
                case 'double':
                    $params[0] .= 'd';
                    break;
            }

            $params[] = $val;
        }
        $method = new ReflectionMethod('mysqli_stmt','bind_param');
        if (!$method->invokeArgs($stmt,$params) && $TrapErrors) {
            //could not bind parameters
            require_once($PathPrefix . 'includes/header.inc');
            prnMsg($ErrorMessage . '<br />' . $db->error,'error', _('Syntax Error'). ' ' .$db->errno);
            if ($debug==1){
                prnMsg($DebugMessage. '<br />' . $query . '<br />' . serialize($params) . '<br />','error',_('Invalid Parameters Passed, bind_param failed.'));
            }
            include($PathPrefix . 'includes/footer.inc');
            exit;
        }
    }
    
    $queryworked = $stmt->execute();
    $_SESSION['LastInsertId'] = $db->insert_id;
    if (!$queryworked && $TrapErrors) {
    
        require_once($PathPrefix . 'includes/header.inc');
        prnMsg($ErrorMessage . '<br />' . $db->error,'error', _('Database Error'). ' ' .$db->errno);
        if ($debug==1){
            prnMsg($DebugMessage. '<br />' . $SQL . '<br />','error',_('Database SQL Failure'));
        }
        if ($Transaction){
            $db->rollback();
            if ($db->errno !=0){
                prnMsg(_('Error Rolling Back Transaction'), 'error', _('Database Rollback Error'). ' ' .$db->errno );
            }else{
                prnMsg(_('Rolling Back Transaction OK'), 'error', _('Database Rollback Due to Error Above'));
            }
        }
        include($PathPrefix . 'includes/footer.inc');
        exit;
    } elseif (isset($_SESSION['MonthsAuditTrail']) && ($db->errno==0 && $_SESSION['MonthsAuditTrail']>0) && ($db->affected_rows)>0){

        $SQLArray = explode($query,' ');
        /*db info works on insert, update, alter table or load data infile, otherwise look for a delete
          if the third element is audittrail, don't log this as it is either DELETE FROM audittrail or INSERT INTO audittrail.  */
        if (($db->info != '' || (strpos($query,'DELETE ') !== false)) && $SQLArray[2] != 'audittrail') {
                $AuditSQL = "INSERT INTO audittrail (userid,
                                    querystring,
                                    params)
                        VALUES('" . trim($_SESSION['UserID']) . "',
                            '" . DB_escape_string($query)  . "',
                            '" . json_encode($params) . "')";

                $AuditResult = $db->query($AuditSQL);
        }
    }
    
    $meta = $stmt->result_metadata();
    //if the SQL statement returns a result set, get the result set, otherwise, return true just like mysqli_query
    if ($meta) {
        //build an associative array of rows with the appropriate column name as keys
        while ($field = $meta->fetch_field())
        {
            $resultset[] = &$row[$field->name];
        }
        $bindResults = new ReflectionMethod('mysqli_stmt','bind_result');
        $bindResults->invokeArgs($stmt,$resultset);
        $result = array();
        //the values for each column have been filled in by bind_result
        while ($stmt->fetch()) {
            foreach($row as $column => $value)
            {
                $c[$column] = $value;
            }
            $result[] = $c;
        }
    } else {
        $result = true;
    }
    $stmt->close();
    
    return $result;
}

If we're planning on rewriting all this code, we should also consider adding support for table prefixes, which would be much appreciated.
Oh, I also realized: we when parameters are passed, it should be in an associative array with named keys, this'll make PDO easier to implement later.
Find all posts by this user
Quote this message in a reply
03-20-2014, 10:29 PM
Post: #34
RE: DB_escape_string
Hi Serakfalcon,

Thanks for the info on the audit trail functionality.

In my prior code I had not provided the supporting functions - here it is in full again with the bindResults(mysqli_stmt $stmt) and refValues($params) functions used by DB_bindquery that I hadn't previously posted.

This is fairly well tried function and based on existing open source code. It has support for php < php 5.3 and also php >= php 5.3 and it seems clean with the refValues function separated out.

The one thing missing for webERP was the corrected/updated code for the audit trail which would need to take the place of the audit trail code in the function below. If left as is we'd only get ? in the sql query in the audit trail. I've not had time this week to look at the audit trail issue so thanks Serakfalcon for your code.

In creating the DB_bindquery I tried to keep the signature as close as possible to the existing DB_query function and thus one reason why it differs from yours.

Personally I prefer to keep the $Conn in the signature for consistency and possible future use, and reduce reliance on globals as much as possible (and $_SESSION vars too but there is a big reliance on them in webERP at the moment). If we don't need the $db global i'd get rid of it.


Here below is the full code including the main DB_bindquery that I previously posted but without the audit trail functionality.

Also the example of insert/edit customer contact below for anyone that want's to try to update existing functions.

Happy to help update the codebase with whatever code is decided on.

Code:
function DB_bindquery($query,
                    &$Conn,
                    $bindvars = null,
                    $ErrorMessage='',
                    $DebugMessage= '',
                    $Transaction=false,
                    $TrapErrors=true)
{

    global $debug;
    global $PathPref;
    global $db;

    $query = filter_var($query, FILTER_SANITIZE_STRING); //sanitize the string
    $stmt = $Conn->prepare($query);

    if (is_array($bindvars) === true) {
        $params = array(''); // Create the empty 0 index
        foreach ($bindvars as $prop => $val) {

            switch (gettype($val)) {
                case 'NULL':
                case 'string':
                    $params[0] .= 's';
                    break;

                case 'integer':
                    $params[0] .= 'i';
                    break;

                case 'blob':
                    $params[0] .= 'b';
                    break;

                case 'double':
                    $params[0] .= 'd';
                    break;
            }

            array_push($params, $bindvars[$prop]);
        }
        call_user_func_array(array($stmt, 'bind_param'), refValues($params));
    }

    $stmt->execute();
    $result = bindResults($stmt);
;
    if (DB_error_no($Conn) != 0 AND $TrapErrors==true){
        if ($TrapErrors){
            require_once($PathPrefix . 'includes/header.inc');
        }
        prnMsg($ErrorMessage . '<br />' . DB_error_msg($Conn),'error', _('Database Error'). ' ' .DB_error_no($Conn));
        if ($debug==1){
            prnMsg($DebugMessage. '<br />' . $query . '<br />','error',_('Database SQL Failure'));
        }
        if ($Transaction){
            $query = 'rollback';
            $Result = DB_query($query,$Conn);
            if (DB_error_no($Conn) !=0){
                prnMsg(_('Error Rolling Back Transaction'), 'error', _('Database Rollback Error'). ' ' .DB_error_no($Conn) );
            }else{
                prnMsg(_('Rolling Back Transaction OK'), 'error', _('Database Rollback Due to Error Above'));
            }
        }
        if ($TrapErrors){
            include($PathPrefix . 'includes/footer.inc');
            exit;
        }
    } elseif (isset($_SESSION['MonthsAuditTrail']) and (DB_error_no($Conn)==0 AND $_SESSION['MonthsAuditTrail']>0) AND ($stmt->affected_rows>0)){
        $SQLArray = explode(' ', $query);
        if (($SQLArray[0] == 'INSERT')
            OR ($SQLArray[0] == 'UPDATE')
            OR ($SQLArray[0] == 'DELETE')) {

            if ($SQLArray[2]!='audittrail'){ // to ensure the auto delete of audit trail history is not logged
                $AuditSQL = "INSERT INTO audittrail (transactiondate,
                                    userid,
                                    querystring)
                        VALUES('" . Date('Y-m-d H:i:s') . "',
                            '" . trim($_SESSION['UserID']) . "',
                            '" . $query . "')";

                $AuditResult = mysqli_query($Conn, $AuditSQL);
            }
        }
    }
    $stmt->close();

    return $result;

}

function refValues($arr)
{
    //Reference is required for PHP 5.3+
    if (strnatcmp(phpversion(), '5.3') >= 0) {
        $refs = array();
        foreach ($arr as $key => $value) {
            $refs[$key] = & $arr[$key];
        }
        return $refs;
    }
    return $arr;
}

function bindResults(mysqli_stmt $stmt)
{
    $parameters = array();
    $results = array();

    $meta = $stmt->result_metadata();

    // if $meta is false yet sqlstate is true, there's no sql error but the query is
    // most likely an update/insert/delete which doesn't produce any results
    if(!$meta && $stmt->sqlstate) {
        return array();
    }

    $row = array();
    while ($field = $meta->fetch_field()) {
        $row[$field->name] = null;
        $parameters[] = & $row[$field->name];
    }

    call_user_func_array(array($stmt, 'bind_result'), $parameters);

    while ($stmt->fetch()) {
        $x = array();
        foreach ($row as $key => $val) {
            $x[$key] = $val;
        }
        array_push($results, $x);
    }
    return $results;
}

Update/Insert customer contact code - ideally with typed bindvars if any doubt about type:
Code:
if (isset($Id) AND ($Id AND $InputError !=1)) {
        $bindvars = array(  ($_POST['ContactName'],
                        $_POST['ContactRole'],
                        $_POST['ContactPhone'],
                        $_POST['ContactNotes'],
                        $_POST['ContactEmail'],
                        $DebtorNo,
                        (int)$Id
                        );
        $sql = "UPDATE custcontacts SET contactname= ?,
                                  role= ?,
                                  phoneno=?,
                                  notes= ?,
                                 email= ?
                    WHERE debtorno =?
                    AND contid=?";
        $msg = _('Customer Contacts') . ' ' . $DebtorNo  . ' ' . _('has been updated');
    } elseif ($InputError !=1) {
        $bindvars= array($DebtorNo,
                        $_POST['ContactName'],
                        $_POST['ContactRole'],
                        $_POST['ContactPhone'],
                        $_POST['ContactNotes'],
                        $_POST['ContactEmail']
                        );
        $sql = "INSERT INTO custcontacts (debtorno,
                                        contactname,
                                        role,
                                        phoneno,
                                        notes,
                                        email)
                VALUES (?,?,?,?,?,?)";
        $msg = _('The contact record has been added');
    }

    if ($InputError !=1) {
        $result = DB_bindquery($sql,$db,$bindvars);
                //echo '<br />' . $sql;

        echo '<br />';
        prnMsg($msg, 'success');
        echo '<br />';
        unset($Id);
        unset($_POST['ContactName']);
        unset($_POST['ContactRole']);
        unset($_POST['ContactPhone']);
        unset($_POST['ContactNotes']);
        unset($_POST['ContactEmail']);
        unset($_POST['Con_ID']);
    }
Find all posts by this user
Quote this message in a reply
03-20-2014, 11:48 PM
Post: #35
RE: DB_escape_string
Hey icedlava, some quick comments about your code,
You notice I have completely different code for the switch(gettype($val)) -> this is because gettype() returns a php type, so it will never return 'blob' for example, and will cause problems if a boolean is passed as a value.

Also, the $SQLArray = explode(' ', $query), while it is in the existing code, is brittle, for example, if there is a space before the statement it will cause the audit trail to not work on that SQL statement, whereas mine still uses some heuristics but errs on the side of over-reporting rather than under-reporting. I also tried to wrap everything inside the same function to avoid proliferation of 'helper' functions that only will be used for this purpose.

My code also has more robust error reporting, a it generates errors if the statement could not be prepared, if the variables could not be bound, and if the statement could not be executed.

Of course, since I use ReflectionMethod the code is strictly PHP 5.3+ but i find it more clear and less likely to cause weird problems than call_user_func_array(), although it's trivial to switch it back if we want to support legacy PHP installations (although seriously, who is still using PHP 5.2 and down, it's 8 years old!)
Find all posts by this user
Quote this message in a reply
03-21-2014, 12:04 AM
Post: #36
RE: DB_escape_string
(03-20-2014 08:31 PM)serakfalcon Wrote:  (On another note, pretty much every DB has a way to log every SQL transaction, so the DB itself could do logging)

What the database logging didn't give us was the webERP user who did the change, which is important info for tracking back on an accounting audit trail.

Thanks
Tim
Quote this message in a reply
03-21-2014, 01:50 AM
Post: #37
RE: DB_escape_string
(03-21-2014 12:04 AM)weberp1 Wrote:  
(03-20-2014 08:31 PM)serakfalcon Wrote:  (On another note, pretty much every DB has a way to log every SQL transaction, so the DB itself could do logging)

What the database logging didn't give us was the webERP user who did the change, which is important info for tracking back on an accounting audit trail.

Thanks
Tim
Ah. that makes a lot of sense. Thanks Smile
Find all posts by this user
Quote this message in a reply
03-21-2014, 11:04 AM
Post: #38
RE: DB_escape_string
Hi serakfalcon,

(03-20-2014 11:48 PM)serakfalcon Wrote:  Of course, since I use ReflectionMethod the code is strictly PHP 5.3+ but i find it more clear and less likely to cause weird problems than call_user_func_array(), although it's trivial to switch it back if we want to support legacy PHP installations (although seriously, who is still using PHP 5.2 and down, it's 8 years old!)

I agree re the php 5.2 and below but I have tried to maintain for php < 5.3 as I'm told we have to still support it.
I have had the discussion before about php versions and there is apparently some argument to keep legacy versions. I have previously suggested we keep a maintained legacy version of webERP to support php <5.3 and allow development in a branch that has a minimum of php 5.3.

Alternatively as php 5.5.10 is the current, php 6 alpha is out and php 5.3 is now legacy, we will have a lot of work, and ugly code to support the lot of them.

I do think, as previously alluded to in this and other threads, a decision needs to be made on this.
Find all posts by this user
Quote this message in a reply
03-21-2014, 01:08 PM (This post was last modified: 03-21-2014 01:09 PM by icedlava.)
Post: #39
RE: DB_escape_string
Hi serakfalcon,

Thanks for your comments.
Quote:You notice I have completely different code for the switch(gettype($val)) -> this is because gettype() returns a php type, so it will never return 'blob' for example, and will cause problems if a boolean is passed as a value.
There are some differences in your code for the switch. The switch I have is meant to gettype - that is the sole function there. Not processing different types of data for the database. That should be imho, done elsewhere and as per my comment on the example AddCustomerContact code, it would be good practice to ensure the bind vars are typed before you pass them.

Some comment on the getype switch:

1. Array/object- I would question why we would try to store an array into a single field in a database table. Similarly I know it has become in some people's mind, general practice to store a serialized/json_encoded object to a single field. There are pros and cons to using json_encode or serialize especially with regard to preserving type.

Whether to use one or other should be thought out - and depends on a number of factors including the database field type it's being saved to, php version, nesting levels in array, encodings, fidelity in 'type' of data that needs to be retained on getting data out, processing required on the data field and any ensuing performance issues, and so on. That is why I would not have the actual typing done for these arrays/objects in this DB_bindquery function which should be dumb in terms of what functioning the data coming in should do. Rather, I would leave it to the function handling the data to process it properly as required, and then pass it to the DB_bindquery function in properly typed bind var array.

2. Boolean - agree on your check for this. However, for the same reasoning above, I would hope the function passing in the data had already processed it accordingly (either directly or via some other function/api) and passed it to the DB_bindquery function.

Having the checks there for the above caters for loose coding, but I'd prefer we pick it up with an error and correct it at source.

(03-20-2014 11:48 PM)serakfalcon Wrote:  the $SQLArray = explode(' ', $query), while it is in the existing code, is brittle, for example, if there is a space before the statement it will cause the audit trail to not work on that SQL statement,
Yes, audit and error code is not my code. I believe generally the whole audit function in webERP could be revised to provide more functionality. I have already some changes here in private branches referred to elsewhere.

Quote:Of course, since I use ReflectionMethod the code is strictly PHP 5.3+ but i find it more clear and less likely to cause weird problems than call_user_func_array(),
You mean probs related to pass by ref or values? It is stricter than call_user_func_array but we are still coding for php < php 5.3. Thus there is the extra function in the code I posted to handle some issues related to that and ensuring consistent var passing in php > 5.3 eg refValue().

Quote:If we're planning on rewriting all this code, we should also consider adding support for table prefixes,
Yes, I think that is a good idea. In fact was confused when i was working on the installer regarding the current 'prefix' used there - it doesn't seem to have any relative value doing what it does at the moment.

All in all, whatever decisions are made there is a bit of work to do Smile

Cheers,
Find all posts by this user
Quote this message in a reply
03-21-2014, 04:51 PM
Post: #40
RE: DB_escape_string
(03-21-2014 01:08 PM)icedlava Wrote:  There are some differences in your code for the switch. The switch I have is meant to gettype - that is the sole function there. Not processing different types of data for the database.
With the current code it doesn't matter if they're typed or not, except in the case that you wanted a number typed as a string or a double as an integer or something like that. PHP variables are typed so of course gettype is going to have something.

What your code actually does seems to be different from what you think it does. It is preparing a string that will be used by bind_param so bind_param knows what type to expect. bind_param expects a subset of MySQL types, whereas gettype() replies with PHP types. That's the discrepancy I'm pointing out. It's worth noting that PDO doesn't have this issue since it typecasts the variables itself.

(03-21-2014 01:08 PM)icedlava Wrote:  1. Array/object- I would question why we would try to store an array into a single field in a database table. Similarly I know it has become in some people's mind, general practice to store a serialized/json_encoded object to a single field. There are pros and cons to using json_encode or serialize especially with regard to preserving type.
Yeah, you're right. I also don't think it's a good idea, it should probably be left out. I only included it for consistency's sake, and used json_encode since most languages have a json parser/encoder of some sort.
With the current code, there is no way to pass 'blob' type but that's probably a good thing. (even though you have 'blob' in the switch it's not a possible output from gettype()).

(03-21-2014 01:08 PM)icedlava Wrote:  Having the checks there for the above caters for loose coding, but I'd prefer we pick it up with an error and correct it at source.

Those checks report an error that identifies the exact source of the problem, and also completely halt the page rendering. It's consistent with the way error handling is done for the existing webERP functions...


(03-21-2014 01:08 PM)icedlava Wrote:  
Quote:Of course, since I use ReflectionMethod the code is strictly PHP 5.3+ but i find it more clear and less likely to cause weird problems than call_user_func_array(),
You mean probs related to pass by ref or values? It is stricter than call_user_func_array but we are still coding for php < php 5.3. Thus there is the extra function in the code I posted to handle some issues related to that and ensuring consistent var passing in php > 5.3 eg refValue().

PHP 5.2 has been unsupported since 2011. Three years ago! PHP 5.3 is also supposed to be reaching end of life sometime this year. I don't think we should enable these sorts of bad practices. If it actually affects anyone, they should update their PHP version. I don't plan on writing code that is deliberately compatible with 5.2 and down. Even if there was a consensus that the project should use PHP 5.2 compatible code, that wouldn't make it a good decision. Since there doesn't seem to be one I'd rather just write PHP 5.3+ code and the project can take it or leave it.
Find all posts by this user
Quote this message in a reply
Post Reply 


Forum Jump:


User(s) browsing this thread: 3 Guest(s)