View Issue Details

IDProjectCategoryView StatusLast Update
0005782Simple:Pressdatabase and sqlpublic2017-07-11 01:58
ReporterYellow SwordfishAssigned ToMr Papa 
PrioritynormalSeverityN/AReproducibilityhave not tried
Status closedResolutionfixed 
Product Version5.8 
Target Version5.8Fixed in Version5.8 
Summary0005782: Quotes in serialised arrays cause the array to be returned empty
DescriptionThis is noticeable - for example - in the custom messages (components) page. The old V5 text appears to be just fine. But - edit the panel - check one of the boxes for example - and then the text is lost because of the quotes around a couple of words.
TagsNo tags attached.
change_log_textQuotes in serialised arrays cause the array to be returned empty
typedefect

Activities

Mr Papa

Mr Papa

2017-03-24 16:02

administrator   ~0019348

No, dont think its related to the option get... seems more tied to the option update... obviously, before you saved, the option was displaying properly...

it seems that when the option is updated and saved to the db, the escaping of the single quotes is lost... where as previously the escaping was preserved...

we did the option update routine from using the wp db update routine to using our sp db update routine, but not sure its there either...

a tough one...
Mr Papa

Mr Papa

2017-03-24 20:08

administrator   ~0019350

have verified that the string is correct as we call:

SP()->DB->update($query)

in the options update... but the escaping is stripped... which causes it to no longer work when fetched since the serialized array is 'broken'...

oddly, tried putting it back to the old $wpdb->update syntax but it failed too... may have to retry that again...
Mr Papa

Mr Papa

2017-03-24 20:20

administrator   ~0019351

yup. my error in recreating (SFOPTIONS vs SPOPTIONS)... it works fine if I use the $wpdb->update call like before... but fails if we use our sp complex db update... the latter strips out the escaping...

so guess next step is figure out why... could be issue across the 6.0 since we are trying to convert all to our db functions...
Mr Papa

Mr Papa

2017-04-03 00:55

administrator   ~0019378

damn Mantis... this wont be as good as the original... ;)

so have tripled confirmed this works fine when using the wp db update function to update with serialized strings... but does not work when using our sp db update function... why?? have dug in bit further...

so this works:

    $result = $wpdb->update(SFOPTIONS, array('option_value' => $newvalue), array('option_name' => $option_name) );

but this does not work:

        $query = new stdClass;
        $query->table = SPOPTIONS;
        $query->fields = array('option_value');
        $query->data = array($value);
        $query->where = "option_name = '$option'";
        $result = SP()->DB->update($query);

the difference seems to come down to that wp update calls wp db prepare, while of course, we do not...

so here is the 5.7.4 query using the wp db update function:

UPDATE `spf_sfoptions` SET `option_value` = 'a:3:{s:13:\"sfpostmsgtext\";s:182:\"Please try and use a title that helps explain the issue. This helps others who may be having the same problem. Titles like \\\'Help Needed\\\' are really not very useful or informative.x\";s:14:\"sfpostmsgtopic\";b:1;s:13:\"sfpostmsgpost\";b:0;}' WHERE `option_name` = 'sfpostmsg'

and here is the 6.0 query using our sp db update function:

"UPDATE spf_sfoptions SET option_value = 'a:3:{s:13:"sfpostmsgtext";s:181:"Please try and use a title that helps explain the issue. This helps others who may be having the same problem. Titles like \'Help Needed\' are really not very useful or informative.";s:14:"sfpostmsgtopic";b:0;s:13:"sfpostmsgpost";b:0;}' WHERE option_name = 'sfpostmsg'"

notice there is a whole lot more slashing going on there... seems the wp db prepare functions array walks through the field/value pairs and does and escape_by_ref() call to each one... which is really just a mysqli real escape... but it makes a big difference...

so what do we need to do?? not sure... but will try playing...
Mr Papa

Mr Papa

2017-04-03 01:00

administrator   ~0019379

betting if we do an esc_sql() on the values before calling the sp db update function it will fix it...

but if so makes me wonder about other places we used wp db calls previously...
Mr Papa

Mr Papa

2017-04-03 01:03

administrator   ~0019380

sure enough... changing out options query to:

        $query = new stdClass;
        $query->table = SPOPTIONS;
        $query->fields = array('option_value');
        $query->data = esc_sql(array($value));
        $query->where = "option_name = '$option'";
        $result = SP()->DB->update($query);

brings it back to working... guess i need to check how many places we did a wp update query in 5.7.4...
Mr Papa

Mr Papa

2017-04-03 01:07

administrator   ~0019381

we will have to watch this on options... but I went and did dozens of other option saves (serialized and not) and seems to be working...
svn

svn

2017-04-03 01:08

administrator   ~0019382

Changeset [15318] by steve on 2017-04-02 21:08:24 -0400 (Sun, 02 Apr 2017)

test issue 0005782 be sure to escape values before calling our update db routine

 Changed Files:

U trunk/sp-api/sp-api-class-spcoptions.php

 Differences:

 http://websvn.simple-press.com/revision.php?repname=Simple:Press&path=%2F&rev=15318
Mr Papa

Mr Papa

2017-04-03 01:12

administrator   ~0019383

there is two other places we call wp db update directly... sp new user email and then in html emails plugin...

both still call it directly... guess we didnt have update function in 5.7.4...

so have not changed those but we need to be aware if we truly get rid of all wp db calls (like we should)...
Yellow Swordfish

Yellow Swordfish

2017-04-03 08:22

administrator   ~0019384

Question: Great investigation... Does the extra esc_sql() just need to be carried out on serialised arrays? If so should we perform a standard test on all insert/updates to test for a serialised array and, if found, perform the esc-sql() on it?
It would introduce a performance hit which would be unwelcome but might make inserts and updates more rugged.
Or - as you say - remember to do this on all calls that contain an array...
Mr Papa

Mr Papa

2017-04-03 13:33

administrator   ~0019388

I think in general we account for this just fine... we need to be careful where we convert old calls to wp db functions to our own sp db functions... we may be losing some escaping in those cases...

we have always said that the caller needs to sanitize/escape... though I have often wondered if we shouldnt convert our db layer to use wp db prepare... should be siimple to do in our layer... the issue might be the extra slashing that could occur... might have to look at all calls and stop escaping...

Issue History

Date Modified Username Field Change
2017-03-22 12:20 Yellow Swordfish New Issue
2017-03-24 16:02 Mr Papa Note Added: 0019348
2017-03-24 20:08 Mr Papa Note Added: 0019350
2017-03-24 20:20 Mr Papa Note Added: 0019351
2017-04-03 00:55 Mr Papa Note Added: 0019378
2017-04-03 01:00 Mr Papa Note Added: 0019379
2017-04-03 01:03 Mr Papa Note Added: 0019380
2017-04-03 01:06 Mr Papa Assigned To => Mr Papa
2017-04-03 01:06 Mr Papa Status new => assigned
2017-04-03 01:06 Mr Papa change_log_text => update
2017-04-03 01:07 Mr Papa Note Added: 0019381
2017-04-03 01:08 svn =>
2017-04-03 01:08 svn Note Added: 0019382
2017-04-03 01:08 svn Status assigned => testing
2017-04-03 01:12 Mr Papa Note Added: 0019383
2017-04-03 08:22 Yellow Swordfish Note Added: 0019384
2017-04-03 13:33 Mr Papa Note Added: 0019388
2017-04-03 16:38 Yellow Swordfish Status testing => closed
2017-04-03 16:38 Yellow Swordfish Resolution open => fixed
2017-04-03 16:38 Yellow Swordfish Fixed in Version => 6.0
2017-04-03 16:38 Yellow Swordfish change_log_text update => Quotes in serialised arrays cause the array to be returned empty
2017-07-11 01:57 Mr Papa Product Version 6.0 => 5.8
2017-07-11 01:58 Mr Papa Fixed in Version 6.0 => 5.8
2017-07-11 01:58 Mr Papa Target Version 6.0 => 5.8