1

So I've created a simple PHP function to 'UPDATE' my MySQL row just by updating the array that is received by PHP.

function db_updateproduct(array $new, array $old = array()) {
    $diff = array_diff($new, $old);
    return 'INSERT INTO table (`'.mysql_real_escape_string(implode(array_keys($diff), '`,`')).'`) VALUES \''.mysql_real_escape_string(implode(array_values($diff), '\',\'')).'\'';
}

...


Update (with Accepted answer)

function db_updateproduct(array $new, array $old = array()) {
    $diff = array_diff($new, $old);
    return 'INSERT INTO `Product` (`'.implode(array_keys($diff), '`,`').'`) VALUES (\''
        .implode(array_map('mysql_real_escape_string', array_values($diff)), '\', \'').'\')';
}

Now...

echo db_updateproduct(array('a' => 'on\'e', 'b' => 'two', 'c' => 'three'));

returns:

INSERT INTO `Product` (`a`,`b`,`c`) VALUES ('on\'e', 'two', 'three')

(As expected/wanted!)

JD Byrnes
  • 783
  • 6
  • 17
  • First shot: Use double quotes for your surounding query string " instead of single quotes '. Do it in implode as well "', '". This should avoid confusing escape sequences. – micfra Mar 16 '12 at 11:03
  • I don't get it. Then why did you put backslashes here? `implode(array_values($diff), '\',\'')` Am I missing something? – rgin Mar 16 '12 at 11:07
  • 1
    **micfra**:, Unfortunately the quotes will still get escaped. – JD Byrnes Mar 16 '12 at 11:23
  • 1
    **rgin**: The backslashed are to use a literal quote inside the quotes. `'\',\''` will only output `','`. It's the mysql_real_escape_string that's doing the damage. – JD Byrnes Mar 16 '12 at 11:24

2 Answers2

6

You can run the escape function on the keys and values with array_map():

$escaped_keys = array_map('mysql_real_escape_string', array_keys($diff)); 

$escaped_values = array_map('mysql_real_escape_string', array_values($diff));

Then you can do your implode() magic on these two arrays.

UPDATE: As @YourCommonSense correctly pointed it out, it does not really make sense to run mysql_real_escape_string() on values that will be used in the query as field names/table names/etc. It correctly escapes \x00, \n, \r, \, ', " and \x1a, but it does NOT escape the backtick, so the query is still vulnerable to attacks.

You should validate the field names (so only the expected names can be used) or even better, use prepared queries (I recommend PDO).

Suggested reading:

Community
  • 1
  • 1
kapa
  • 77,694
  • 21
  • 158
  • 175
  • Didn't think of that, but it sounds great. Thanks mate! I'll give it a go now! – JD Byrnes Mar 16 '12 at 11:21
  • @YourCommonSense Looking at this method, you don't really know where the keys are coming from. If they come directly from a form, the keys can be manipulated (thus should be escaped, if validation did not take place - we don't know this and it is out of the question's scope). I don't feel this is worth a downvote, but I understand it is your decision only how you use them. – kapa Mar 16 '12 at 17:06
  • oh really? imagine I manipulated these keys hard. say, made `c` one looks like "c`) VALUES(NULL,NULL,NULL);DROP TABLE USERS; --" What will your escaping do with it? – Your Common Sense Mar 16 '12 at 17:14
  • @YourCommonSense Got it, it is trivial, you're right. I will correct my answer. – kapa Mar 16 '12 at 17:23
  • this is not that trivial. 95% of local folks do believe that escaping does some magic "sanitization", making whatever data "safe". so, your answer only follows this very common misbelief. – Your Common Sense Mar 16 '12 at 17:27
  • @YourCommonSense I did not pay enough attention, should not write answers on SO when too drunk. Thanks. – kapa Mar 16 '12 at 17:36
  • Hehe, I like the desc though :) – Your Common Sense Mar 16 '12 at 18:16
  • @YourCommonSense Thanks. There was no point. (see next line) The Title is 'This query is over-escaped!' though :P – JD Byrnes Mar 16 '12 at 23:48
  • @bazmegakapa The keys are safe (they're coming from a safe source) – JD Byrnes Mar 16 '12 at 23:50
1

As a matter of fact, doing mysql_real_escape_string on the array keys is an example of absolutely useless action.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • As a matter of fact, this is not an answer :). – kapa Mar 16 '12 at 17:06
  • Yes, it is useless. I'm not really sure why I tried to sanitize it. But... this is not an answer. – JD Byrnes Mar 16 '12 at 23:45
  • 1
    i will up vote it anyway since such a good point is worth it. It would be much better though if you provided an explanation of why's that (as you did in the comment above) – mkk Mar 17 '12 at 00:09