#418 UPDATE single row with varchar key: empty where clause

closed-fixed
General (123)
7
2012-02-28
2012-01-03
Dirk Kraemer
No

Version: phpPgAdmin-5.0.3-1.el5 (CentOS release 5.7)

We observed a problem when updating a row
in a table with a varchar key column.
The where clause in the generated UPDATE statement is
totally missing an the statement would update
all rows instead of only the current one.

Some details in following example:

We have a table with a single varchar key column
'pcname'. We want to update a row with value of
'msh100' with phppgadmin.

The generated page where we can see and edit
the columns values contains a hidden field with
the key values:

<input type="hidden" name="key" value="a:1:{s:6:&quot;pcname&quot;;s:6:&quot;msh100&quot;;}" />

This looks like a serialzed version of

$key = array ("pcname" => "msh100");

However: Occurrences of quotes '"' are replaced by 'quot;'.

It seems that the replacement of '"' with '&quot;' make the
unserialization fail. This is done e.g. in line 173,174 of display.php:

173 $status = $data->editRow($_POST['table'], $_POST['values'], $_POST['nulls'],
174 $_POST['format'], $_POST['types'], unserialize($_POST['key']));

So the result of unserialize is not an array and the where-clause
is not

where pcname='msh100'

but empty.

This is just a quick guess. Don't know internals of
phppgadmin enough to give more hints. Hope this
report is sufficient.

Thanks and regards

Dirk

Discussion

  • Hello,

    This is a pretty ugly bug, I don't understand why we never found/heard about it :(

    I'll work on this tonight and try to fix this as soon as possible.

    Thank you for this report !

     
    • priority: 5 --> 7
    • assigned_to: nobody --> ioguix
     
    • status: open --> pending
     
  • So I tried to reproduce the bug without success (PostgreSQL 9.1.2, PPA 5.0.3, PHP 5.3.3, lighttpd 1.4.28 and FF 8.0).
    I used a single column table, with type 'text' and a PK on it. Update on a row went fine.

    «It seems that the replacement of '"' with '&quot;' »

    This is normal, we must escape the serialized array before putting it in a HTML attribute. If quotes were not escaped, we would break the html code. However, data are sent non-escaped by any browser (usually).

    Do you have any other information abou tthis bug which would lead me to the real bug ?

     
  • According to the PHP doc, unserialize should rise a E_NOTICE when it fails. Could you check your httpd log files or try to setup your php.ini to catch it ?

    see: http://php.net/unserialize

    thank you.

     
  • Dirk Kraemer
    Dirk Kraemer
    2012-01-05

    First of all: Thank you very much for searching.
    I tried to reproduce the error and I think i found
    a hint:

    Indeed the problem does not occur if there is only
    a normal value in key column. So my example was not
    sufficient, sorry for that.

    But if there is a newline character at the end and try to update
    we have a problem.

    I put the value 'mypc<newline>' into key column.

    Hidden field is now

    <input type="hidden" name="key" value="a:1:{s:6:&quot;pcname&quot;;s:5:&quot;mypc
    &quot;;}" />

    When we click save button we get:

    Message in web interface:

    SQL error:

    FEHLER: doppelter Schlüsselwert verletzt Unique-Constraint »spss_pkey«
    (double key value violates Unique constraint)

    In statement:

    UPDATE "public"."spss" SET "pcname"='mypc
    ', "einrichtungskennzeichen"='RZS ', "nummer"='69', "lizenz_beginn"='2012-01-05', "lizenz_ende"='2012-01-06', "spss_version"='20 ', "win_version"='- ', "rechnungsdatum"=NULL, "bemerkung"=''

    The same error message can be found within postgres logfile.
    The where clause is missing in update statement.

    We also find errors in web server's log file complaining about
    errors in unserialize function and invalid argument for foreach.
    I will post these separately in a small file.

    Sorry, i had not noticed that the row which led to the error had
    the newline character in the key column, while my example had not.
    I should have checked my bug report better before posting.

     
  • Dirk Kraemer
    Dirk Kraemer
    2012-01-05

    • status: pending --> open
     
  • Dirk Kraemer
    Dirk Kraemer
    2012-01-05

    PHP Errors from Apache logfile

     
  • Thank you for your investigation !

    So I'm now able to reproduce your bug and found what happen, at least on my side. Using a string with a newline, when serializing it, the newline was "\n". But when POST-ing it to the serveur, this newline becomes "\r\n", I guess because of the HTTP protocol.

    So on the other side, when unserializing the array, this string is one char longer. As the serialization algorithm keeps track of each string parameters size, the unserialize function fail because the value sounds different. I have been able to validate that adding the following line in the beginning of the doEditRow function:

    $_REQUEST['key'] = preg_replace('@\r\n@', "\n", $_REQUEST['key']);

    which fix the bug.

    So the trick is to encode newlines so they are not replaced on the way. So far, I have been able to fix this bug combining (un)serialize with base64_encode/decode OR urlencode/decode. I'll think a bit more about the best solution, and look for some other places in the code which might be impacted as well, then produce a bug fix.

    Thank you for your help and report, stay tuned for the fix soon !

     
    • status: open --> closed-fixed
     
  • Dirk Kraemer
    Dirk Kraemer
    2012-02-28

    Thank your for that fix.
    I changed the affected lines within display.php in out current installation.
    Updates seem to work correctly now.

    However, we still seem to have problems when trying to delete rows.

    Shouldn't we add similar patches to

    function deDelRow

    echo "<input type=\"hidden\" name=\"key\" value=\"", htmlspecialchars(serialize($_REQUEST['key'])), "\" />\n";
    ......
    $status = $data->deleteRow($_POST['table'], unserialize($_POST['key']));

     
  • Dirk Kraemer
    Dirk Kraemer
    2012-02-28

    Yes, now it seems to work correctly.

    Thanks again for this fix and also for
    your other work in this project.