Checkout: Comments with htmlspecialchars handling

**lat9** · 23 Jul 2016, 12:49 PM

I've searched a while and haven't seen a previous report of this issue.

In a vanilla 1.5.5a Zen Cart with the demo products loaded, add a product to your cart and start the checkout process.

On the checkout_shipping page, enter the following comments:

Code:

Here's some "comments" & other stuff

Continue to checkout_payment, see that the comments are carried over from the previous page and continue to the checkout_confirmation page.

See now that your comments are displayed as:

Code:

Here's some &quot;comments&quot; &amp; other stuff

which, unfortunately, is also how those comments get stored to the order in the database.

**lat9** · 23 Jul 2016, 01:05 PM

That processing is the result of line #49 (ZC 1.5.5a) in /includes/modules/pages/checkout_confirmation/header_php.php

Code:

$_SESSION['comments'] = zen_output_string_protected($_POST['comments']);

The code was changed based on the Trustwave Security Patch. My initial thought is to change that to call zen_clean_html instead, since it's a <script></script> injection that the code's looking to prevent.

**lat9** · 27 Jul 2016, 01:48 PM

If anyone else has run into this issue, here's a teeny script that will turn those & and " items back to their & and " values:

Code:

<?php
require ('includes/application_top.php');

$status_records = $db->Execute (
    "SELECT orders_status_history_id, comments FROM " . TABLE_ORDERS_STATUS_HISTORY . "
      WHERE comments LIKE '%&amp;%'
         OR comments LIKE '%&quot;%'"
);

while (!$status_records->EOF) {
    $comments = $db->prepare_input (str_replace (array ('&amp;', '&quot;'), array ('&', '"'), $status_records->fields['comments']));
    $db->Execute ("UPDATE " . TABLE_ORDERS_STATUS_HISTORY . " SET comments = '$comments' WHERE orders_status_history_id = " . $status_records->fields['orders_status_history_id'] . " LIMIT 1");
    $status_records->MoveNext ();
}

echo "Processing completed. " . $status_records->RecordCount () . " records were updated."; 

require ('includes/application_bottom.php');

Just create a file containing the code above, place the file in the root of your store's file system, run it and then delete the file.

For example, if you name the file fix_orders_status.php, you'll run it as www.example.com/fix_orders_status.php.

**lat9** · 2 Aug 2016, 03:53 PM

Originally Posted by lat9

That processing is the result of line #49 (ZC 1.5.5a) in /includes/modules/pages/checkout_confirmation/header_php.php

Code:

$_SESSION['comments'] = zen_output_string_protected($_POST['comments']);

The code was changed based on the Trustwave Security Patch. My initial thought is to change that to call zen_clean_html instead, since it's a <script></script> injection that the code's looking to prevent.

Unfortunately, the zen_clean_html function's "over-achieving", too, since it's stripping all the CR/LF sequences from the comments, rendering:

Code:

Here's some "comments" &
other stuff

into

Code:

Here's some "comments" & other stuff

Perhaps, it would be appropriate to add another input to the function (present in /includes/functions/functions_general.php) to specify that CR, LF and TAB characters should remain:

Code:

function zen_clean_html($clean_it, $extraTags = '', $keep_new_lines = false) {
    if (!is_array($extraTags)) $extraTags = array($extraTags);

    // remove any embedded javascript
    $clean_it = preg_replace('#<script(.*?)>(.*?)</script>#is', '', $clean_it);

    if (!$keep_new_lines) {
        $clean_it = preg_replace('/\r/', ' ', $clean_it);
        $clean_it = preg_replace('/\t/', ' ', $clean_it);
        $clean_it = preg_replace('/\n/', ' ', $clean_it);

        $clean_it= nl2br($clean_it);

        // update breaks with a space for text displays in all listings with descriptions
        $clean_it = preg_replace('~(<br ?/?>|</?p>)~', ' ', $clean_it);
    }

// temporary fix more for reviews than anything else
    $clean_it = str_replace('<span class="smallText">', ' ', $clean_it);
    $clean_it = str_replace('</span>', ' ', $clean_it);

// clean general and specific tags:
    $taglist = array('strong','b','u','i','em');
    $taglist = array_merge($taglist, (is_array($extraTags) ? $extraTags : array($extraTags)));
    foreach ($taglist as $tofind) {
      if ($tofind != '') $clean_it = preg_replace("/<[\/\!]*?" . $tofind . "[^<>]*?>/si", ' ', $clean_it);
    }

// remove any double-spaces created by cleanups:
    $clean_it = preg_replace('/[ ]+/', ' ', $clean_it);

// remove other html code to prevent problems on display of text
    $clean_it = strip_tags($clean_it);
    return $clean_it;
  }

Once that function's been updated, that line in the checkout_confirmation header file would be changed to:

Code:

$_SESSION['comments'] = zen_clean_html($_POST['comments'], '', true);

**DrByte** · 11 Oct 2016, 08:50 PM

What if we just run strip_tags() on it? ... since that removes all html chars, but doesn't mess with line-breaks.

**lat9** · 12 Oct 2016, 01:08 PM

That was my first thought, but I was unsure if the zen_clean_html function didn't "do more".

**DrByte** · 13 Oct 2016, 06:37 PM

It seems that the zen_clean_html is indeed doing more than it needs to. So, if you're regularly running into this, using strip_tags would do it.

But it's likely that the zen_output_string_protected should not be run against the variable, which gets stored to db, but rather within the template upon output, for display purposes.

Additionally, it seems this is fine in v160.