Printable View
When using edit orders Zen Cart v1.5.4
I'm getting the following error, edit orders does not save properly so not sure is this something edit orders needs to address or is this an issue with the AdminRequestSanitizer.php file?
[18-Mar-2016 09:23:18 America/New_York] PHP Warning: htmlspecialchars() expects parameter 1 to be string, array given in /var/admin/includes/classes/AdminRequestSanitizer.php on line 319
For capacity reasons the sanitizer could not be tested with all plugins. So, this needs investigation, and probably an update to Edit Orders to address it.
Which feature of Edit Orders were you using to trigger this? It appears to be a problem with something that would have multiple level depths, such as checkboxes or multiple levels of languages within groups.
In addition to describing which feature of EO you were using, you could also obtain more info about what data needs parsing differently by making a small TEMPORARY code change:
In that Admin Sanitizer class file, insert a new line around line 330, as shown here:And then it should dump out to your admin screen the values it's trying to process.Code:foreach ($_GET as $key => $value) {
if (!in_array($key, $getToIgnore)) {
if (is_array($value)) {
foreach($value as $key2 => $val2){
if (is_array($val2)) die('Value of ['.$key2.'] found to be array: <pre>' . print_r($value, true));
$_GET[$key][$key2] = htmlspecialchars($val2);
}
} else {
Hello DrByte,
I tried to edit an order using edit orders and the onetime discount mod to show a a refund that I issued to a customer. Once I saved it, the product on the order for whatever reason was deleted and the refund that was entered was not saved. I then tried to add the product that was deleted back and save it and it would not save it just kept generating that error. I had to remove the AdminRequestSanitizer.php and revert back to the original init_sanitize.php file for edit orders to start working again.
Ok will do it now and report back.
Hi
As a temporary fix
see https://docs.zen-cart.com/Developer_...t-sanitization
I'll take a look at the plugin to see if there is a better fix.
Ok DrByte,
I applied your code change, but it did not display anything to me. Something happens when I edited the the order and tried to give a discount using edit orders/onetime discount it says "Success: Order has been successfully updated" however the discount does not save and it removes the product that was purchased on the order leaving only the shipping.
Same error log is generated:
[18-Mar-2016 15:42:36 America/New_York] PHP Warning: htmlspecialchars() expects parameter 1 to be string, array given in /var/admin/includes/classes/AdminRequestSanitizer.php on line 319
wilt just tried your temp solution and it does work.
Argh, cuz I gave you the wrong lines :(Quote:
Originally Posted by marcopolo
At line 319 insert the new line shown:Code:foreach ($_POST as $key => $value) {
if (!in_array($key, $postToIgnore)) {
if (is_array($value)) {
foreach($value as $key2 => $val2){
if (is_array($val2)) die('Value of ['.$key2.'] found to be array: <pre>' . print_r($value, true));
$_POST[$key][$key2] = htmlspecialchars($val2);
}
} else {
$_POST[$key] = htmlspecialchars($value);
}
}
}
foreach ($_GET as $key => $value) {
Below is the output however I do not think it's executing all the way as nothing is saving.
Value of [127213] found to be array:
Code:Array
(
[127213] => Array
(
[qty] => 1
[name] => Test Item
[model] => G10
[tax] => 2
[final_price] => 12.00
)
)
also not error logs are being generated.
Right. It's just to get an idea of what data needs treatment.Quote:
Originally Posted by marcopolo
Thanks, that info is helpful.
Remove those lines I suggested, as they're just for collecting data. Use wilt's temporary suggestion until a proper fix is determined.
Ok thanks
Does it make sense just to add a third layer?
As far as I can tell this fixes the log.Code:foreach ($_POST as $key => $value) {
if (!in_array($key, $postToIgnore)) {
if (is_array($value)) {
foreach($value as $key2 => $val2){
if (is_array($val2)) {
foreach($val2 as $key3 => $val3){
$_POST[$key][$key2][$key3] = htmlspecialchars($val3);
}
} else {
$_POST[$key][$key2] = htmlspecialchars($val2);
}
}
} else {
$_POST[$key] = htmlspecialchars($value);
}
}
}
Edit orders fabricates the order totals in a multilayered structure. It probably does other things like this too.
Code:Array
(
[0] => Array
(
[code] => ot_combination_discounts
[title] => Combination Discounts :
[value] => 3.0000
)
[1] => Array
(
[code] => ot_fuelsurcharge
[title] => Fuel Surcharge:
[value] => 8.5753
)
[2] => Array
(
[code] => ot_shipping
[shipping_module] => flat
[title] => Regular Shipping (Basic shipping included):
[value] => 0.0000
)
[3] => Array
(
[code] => ot_snqd
[title] =>
[value] =>
[shipping_module] =>
)
)
A recursive approach could be taken instead of "planning" for depth... Also, it seems that since ZC 1.5.1 the use of htmlspecialchars has been expanded to include other "directives", should those not be added to that code instead of letting things go as defaulted?
Hi
I do have a pending commit that uses recursion.
https://github.com/zencart/zencart/pull/886/files
Wilt's fix has been merged into the v155 branch on github ... and is now part of core code since the 03-29-2016 zip of v155.
Please run it with Edit Orders. I think the only potential "issue" with it is that it might mangle any HTML in product names when editing one of those in an order.
Add this code on top of edit_order.phpQuote:
Originally Posted by marcopolo
PHP Code:define('DO_STRICT_SANITIZATION', false);
This is a poor coding recommendation at this point especially with an overall fix provided, further it goes against the information provided at the Developer's Documentation area specifically on the use of this define.Quote:
Originally Posted by cvhainb
Fixes have been posted; apply them. DO NOT turn off the XSS patches.Quote:
Originally Posted by cvhainb
As I posted in the Edit Orders support thread (https://www.zen-cart.com/showthread....92#post1307492), the fix is necessary but insufficient. If you use EO and add a product with attributes, the issue persists.Quote:
Originally Posted by swguy
i have watched this thread for a bit. i find it amusing/sad that the dev team is forced to change CORE code to make it work with a plug-in; not the other way around.
it would not be so amusing to me if the plugin was good. but as i have expressed in the past, i feel EO is a necessary evil; it is a bloated add-on that sucks to debug; and frankly i do not agree with many of the design choices made in its coding. and to the people who choose to maintain and help others with this plugin, you are far braver than me.
PLEASE PLEASE PLEASE include this functionality in the core for v1.6 so that we can put EO out of its misery.
Edit Orders 4 adding a product with attributes doesn't seem to work perfectly whether or not you have this change. I have not been able to get the attributes to show up in the order either way.Quote:
Originally Posted by lat9
Changes like this happen because volunteers do the work and submit PRs. Perhaps you?Quote:
Originally Posted by carlwhat
perhaps... i am trying to spend more time looking as well as following the activity... still have other things on my plate...Quote:
Originally Posted by swguy
I've got a bunch of clients using EO 4.1.4 on stores with heavy attribute usage and there's never been a problem (before). Perhaps you could add a post to the EO support thread mentioning that you're seeing this behavior and we could help you correct your installation.Quote:
Originally Posted by swguy
Is your posting meant to indicate that all's been done that's going to be done to end EO's misery in the presence of this Zen Cart 1.5.5 change?
Your faith in my ability to prognosticate is tragically misplaced. Anyone, at any time, is free to post a patch, submit a PR, update a mod, or submit a new mod. Impossible to predict, the future is.Quote:
Originally Posted by lat9
ATTENTION ALL DEVELOPERS: Please feel free to jump into the fray if you think you have a solution.
Appreciate the offer. Let me dup the issue on a pristine 1.5.5 + EO test cart to be sure it's not a bad interaction with another change.Quote:
Originally Posted by lat9
Meanwhile, anything you could do relating to the "over preparation" issue in EO as described in the thread below, it would be great.
https://www.zen-cart.com/showthread....94#post1307494
Well, what's somewhat interesting about the situation is that the issue being addressed, is the security of the admin side which through "normal" routes would not be accessible to the problem being addressed by the sanitizer. But regardless of the one plugin, there are likely others that are similarly affected. The effort put forth just goes to show the level of concern to security and applying it to all aspects of ZC. I wouldn't consider addressing this as just trying to get EO to work, but security to work within the routine processes of PHP programming. To date that I know of there has not been a "you may not use the following code/data formats with ZC because ZC doesn't know how to handle that" instruction...Quote:
Originally Posted by carlwhat
Doesn't the foreach depend on $item being array, but it seems that there is no check either upon entry into traverseStricSanitize or before/as a part of calling traverseStricSanitize. Ie, is it possiblethat $item could be empty I guess? I can't seem to think where either $_GET or $_POST would specifically equal a value and not some formof an array... Hmmm... Now that I say that though... Use of Super Globals while watching the store front, add a product to the cart, and $_POST which at some point had data is now "empty". Wondering if the issue is similar/related... Otherwise if $item is not an array, could throw it into one?Quote:
Originally Posted by wilt
I haven't been able to test related to this, but it has me curious...
The traverseStricSanitize function passes $item by reference, and for the initial entry point uses either $_POST or $_GET and these will always be arrays whether empty or not.
The problem is that something like Edit Orders passes a multi dimension array, where child entries need their own sanitization, rather than using the default strict sanitization and the current admin sanitizer class doesn't support that.
I am working on code at the moment that will allow for defining validation at a more granular level, and hope to have a PR soon
Quote:
Originally Posted by mc12345678
This is not what is happeningQuote:
i have watched this thread for a bit. i find it amusing/sad that the dev team is forced to change CORE code to make it work with a plug-in; not the other way around.
We are not going to put code into core that adds sanitization for specific Edit Order code parameters
Although I might suggest how EO might work better with core.
The problem is that currently core code does not have sufficient hooks to allow some thing like EO to use our sanitizer correclty.
Thats not EO's problem
Yeah, I saw the by reference style and when the original reason of not working was identified (multi-dimension array), I was half expecting something along the lines of the following (doesn't use pass-by-reference) and not trying to derail existing coding path:Quote:
Originally Posted by wilt
It's initiated by passing the array and the number of initial components of the main array, probably could be simplified that small amount further to not need to pass the number of array elements, but when that solution was found for the problem at hand, that was that as it worked and I didn't want to mess it up. :)Code:function return_array_combinations($arrMain, $intVars, $currentLoop = array(), $currentIntVar = 0) {
$arrNew = array();
for ($currentLoop[$currentIntVar] = 0; $currentLoop[$currentIntVar] < sizeof($arrMain[$currentIntVar]); $currentLoop[$currentIntVar]++) {
if ($intVars == $currentIntVar + 1) {
$arrNew2 = array();
for ($i = 0; $i<$intVars;$i++) {
$arrNew2[] = $arrMain[$i][$currentLoop[$i]]; // This is a place where an evaluation could be made to do something unique with a single component that is to be assigned to a single combination as this assigment is for one of the arrays to be combined for one record. If the goal would be not to add anything having this array, then could call continue 2 to escape this for loop and move on to the next outer for loop. If just want to not add the one array/component to the combination then place the above assignment so that it is bypassed when not to be added.
}
if (zen_not_null($arrNew2) && sizeof($arrNew2) > 0) { // This is a place where an evaluation could be made to do something unique with an array or component as this assigment is one of all arrays combined for one record. //Still something about this test doesn't seem quite right, but it's the concept that is important, as long as there is something to evaluate/assign that is not nothing, then do the assignment.
$arrNew[] = $arrNew2;
}
} else {
$arrNew = array_merge($arrNew, return_array_combinations($arrMain, $intVars, $currentLoop, $currentIntVar + 1));
}
}
return $arrNew;
}
It doesn't have/use the additional feature of logging/identifying the "starting" point of the "unrecognized" key, but I thought I would offer some code that has been developed to dig into the depths of an array of an array of an array, etc... recursively.
This was developed to accomplish something similar to the array of array situation, though the conditions are/were a little different as far as the earlier build of action based on condition to perform the next/same action with the next group... The problem being addressed/resolved was:
ultimately, every combination of the original array is addressed; however, the original variable was a single depth array each item having one set of keys with a matching value, but ultimately an array of arrays is built. Don't know if it helps at all... I'm not in a position to tie into the file being addressed here.Code:if ($intVars >= 1) {
//adds array combinations
// there are X variables / arrays
// so, you need that many arrays
// then, you have to loop through EACH ONE
// if it is the LAST variable / array
// you need to add that variable / array VALUE
// and ALL PREVIOUS VALUES to the multi-dimensional array
// below supports up to 7 variables / arrays
// to add more, just copy and paste into the last for loop and go up from $n is the last one
for ($i = 0;$i < sizeof($arrMain[0]);$i++) {
if ($intVars >= 2) {
for ($j = 0;$j < sizeof($arrMain[1]);$j++) {
if ($intVars >= 3) {
for ($k = 0;$k < sizeof($arrMain[2]);$k++) {
if ($intVars >= 4) {
for ($l = 0;$l < sizeof($arrMain[3]);$l++) {
if ($intVars >= 5) {
for ($m = 0;$m < sizeof($arrMain[4]);$m++) {
if ($intVars >= 6) {
for ($n = 0;$n < sizeof($arrMain[5]);$n++) {
if ($intVars >= 7){
for ($o = 0; $o < sizeof($arrMain[6]); $o++) {
$arrNew[] = array($arrMain[0][$i], $arrMain[1][$j], $arrMain[2][$k], $arrMain[3][$l], $arrMain[4][$m], $arrMain[5][$n], $arrMain[6][$o]);
}
} else {
$arrNew[] = array($arrMain[0][$i], $arrMain[1][$j], $arrMain[2][$k], $arrMain[3][$l], $arrMain[4][$m], $arrMain[5][$n]);
}
}
} else {
$arrNew[] = array($arrMain[0][$i], $arrMain[1][$j], $arrMain[2][$k], $arrMain[3][$l], $arrMain[4][$m]);
}
}
} else {
$arrNew[] = array($arrMain[0][$i], $arrMain[1][$j], $arrMain[2][$k], $arrMain[3][$l]);
}
}
} else {
$arrNew[] = array($arrMain[0][$i], $arrMain[1][$j], $arrMain[2][$k]);
}
}
} else {
$arrNew[] = array($arrMain[0][$i], $arrMain[1][$j]);
}
}
} else {
$arrNew[] = array($arrMain[0][$i]);
}
}
So one of the reasons Edit Orders is broken with reference to the sanitizer
is when attempting to add a new product to the order(as has been mentioned elsewhere in the thread) .
It breaks because EO uses a post parameter called 'id' to pass a multi-dimensional array.
However the sanitizer already adds the 'id' parameter to a Sanitization Group that converts the parameter to an integer, which obviously destroys the
array structure in EO.
I mentioned in the documentation for the Sanitizer the need for developers to be careful with parameter naming.
However, I also realize that forcing developers to have to rewrite code to address parameter naming could be onerous and likely to introduce bugs initially, and could delay store owners in adopting v1.5.5, if popular contributions are not updated.
As I mentioned previously, I am working on a rewrite of the sanitizer to make it more granular, and I’m also exploring some changes to the data structures that would allow contributions to override sanitizing of a parameter on a per page basis.
@wilt, I'm in the process of incorporating various bug-fixes into Edit Orders. Would changing that 'id' parameter to have some other name be enough to "pass" the sanitizer?
I'll answer my own question: Yes.Quote:
Originally Posted by lat9
I've got an updated version of EO on GitHub (https://github.com/lat9/edit_orders) that includes that work-around.
Hi.
In one way, yes it would.
Changing the parameter name to something that does not already have a sanitizer assigned will force that parameter to be sanitized
by the filterStrictSanitizeValues (if strict sanitize has not been disabled)
For the id parameter (renamed attr_info in your github) this is probably OK.
However here's another gotcha
In edit_orders, I can edit the product name and on submit this uses the
$_POST[update_products] field
Because this has no sanitizer ascribed to it, it gets the recursive filterStrictSanitizeValues applied
However we normally allow some html in product names, which filterStrictSanitizeValues will break
Quote:
@wilt, I'm in the process of incorporating various bug-fixes into Edit Orders. Would changing that 'id' parameter to have some other name be enough to "pass" the sanitizer?
Quote:
Originally Posted by lat9
@wilt, thanks for the update. In thinking about the id => attr_info name, it occurred to me that there might be additional gremlins that are brought out in that renaming, since EO tries to keep in the storefront naming (i.e. the attributes are POST'd in the id variable) there might be additional issues with that renaming.
I'll see what I can do using the sanitizer overrides ...
... reading the documentation, it looks like I'm :censored:Quote:
Originally Posted by lat9
Since the id parameter is already sanitized, is there no hope?Code:if you are writing a plugin and use a parameter name that already exists in Zen Cart that parameter will be sanitized according to the group it is already assigned to in core code
Another question: How do array variables get registered with the sanitizer? Do I "register" each sub-variable name? Is there a sanitizer group to define an array variable?
HiQuote:
Originally Posted by lat9
So yes there is hope :)
I would forget about changing id -> attr_info, especially if it is likely to break other code.
The new version of the adminSanitizer class i'm currently working on. will allow you to override sanitization on a per page basis. So even if we give a
general sanitizer for the id parameter, you will be able to override that for edit orders.
So at the moment there is no way of defining sanitization except at a top level basis,Quote:
Originally Posted by lat9
so I can't add sanitizers at a sub level at the moment
However again, I'm currently working on a MUTLI_DIMENSIONAL sanitizer that allows you to define something like
PHP Code:$group = array(
'id' => array('sanitizerType' => 'MULTI_DIMENSIONAL',
'method' => 'post', 'pages' => array('edit_orders'), 'params' => array('id'=>'CONVERT_INT', 'name'=>'WORDS_AND_SYMBOLS_REGEX')));
at the moment this still doesn't let you recurse even deeper.
so if you look at edit orders it creates a post array
PHP Code:[update_products] => Array
(
[13] => Array
(
[qty] => 1
[name] => Microsoft IntelliMouse Explorer
[onetime_charges] => 0.0000
[attr] => Array
(
[3] => Array
(
[value] => 11
[type] => 0
)
)
[model] => MSIMEXP
[tax] => 0
[final_price] => 70.95
and MULTI_DIMENSIONAL doesn't allow you to define a deep sanitizer for [attr]
what I want to happen is to allow you to define MUTLI_DIMENSIONAL within an outer MUTLI_DIMENSIONAL, but as you can imagine, that involves
some wonderful recursive structures/code.
Oh boy! Recursion! That's always fun ... especially to debug.
It also reminds me of one of my favorites (seen in a tongue-in-cheek document index):
Recursion: See recursion.
:DQuote:
Originally Posted by lat9
Hoping to finish the code for this tonight.
The data structure for defining the sanitization would be
It should be noted that you don't necessarily have to go to this level of sanitizing, but I feel it should be available.PHP Code:$group = array(
'update_products' => array(
'sanitizerType' => 'MULTI_DIMENSIONAL',
'method' => 'post',
'pages' => array('edit_orders'),
'params' => array(
'update_products' => array('sanitizerType' => 'CONVERT_INT'),
'qty' => array('sanitizerType' => 'CONVERT_INT'),
'name' => array('sanitizerType' => 'WORDS_AND_SYMBOLS_REGEX'),
'onetime_charges' => array('sanitizerType' => 'SIMPLE_ALPHANUM_PLUS'),
'attr' => array(
'sanitizerType' => 'MULTI_DIMENSIONAL',
'params' => array(
'attr' => array('sanitizerType' => 'CONVERT_INT'),
'value' => array('sanitizerType' => 'CONVERT_INT'),
'type' => array('sanitizerType' => 'CONVERT_INT')
)
),
'model' => array('sanitizerType' => 'WORDS_AND_SYMBOLS_REGEX'),
'tax' => array('sanitizerType' => 'SIMPLE_ALPHANUM_PLUS'),
'final_price' => array('sanitizerType' => 'SIMPLE_ALPHANUM_PLUS'),
)
)
);
My most recent changes are here
https://github.com/zcwilt/zc-v1-seri...9ff41070bab641
however that doesn't yet have code to do the MULTI_DIMENSIONAL recursion.
Looks good, wilt; that's how I was envisioning the configuration. I'll watch for your updates.
Quote:
Originally Posted by lat9
Infinite Loop: See Loop, Infinite
Loop, Infinite: See Infinite Loop
So I think I am close to a finished solution now
https://github.com/zcwilt/zc-v1-seri...aee0cdd95199cb
Just a couple of things left to do
Add some more unit tests
and update the Documentation for the Admin Sanitizer
It's looking good, so far! I'll keep at it and let you know if I come across anything.
I'm working on updated documentation, and probably some expanded tests.
Will post here once done.
@wilt, is there a way to stop the unwanted htmlentities' conversions? If I order the product A Bug's Life "Multi Pak" Special 2003 Collectors Edition
and then edit that order (simply pressing the "Update" button), the name changes to A Bug's Life "Multi Pak" Special 2003 Collectors Editi ... with the double-quotes converted to " and the name getting truncated due to the additional characters.
The same thing happens if I enter a text attribute that uses special characters, e.g. Here's some text … gets converted to Here's some text &hellip; -- and it just gets worse each time that order is updated since each & is converted to &.
Have you defined any extra sanitizers.
I'm about to update the docs regarding this.
Quote:
Originally Posted by lat9
Just to be clear here.
If you want to test latest code, you need to pull in the changes from my https://github.com/zcwilt/zc-v1-seri...itizer-updates branch
and there are 3 files
admin/includes/auto_loaders/config.adminSanitize.php
admin/includes/classes/AdminRequestSanitizer.php
admin/includes/init_includes/init_sanitize.php
Even pulling these in won't fix edit_orders as you then need to add your own sanitizers
as a simple test I created
/admin/includes/extra_datafiles/edit_orders_sanitize.php that contained
and that fixed htmlentities problemsPHP Code:<?php
/**
* Created by PhpStorm.
* User: wilt
* Date: 07/04/16
* Time: 20:45
*/
$sanitizer = AdminRequestSanitizer::getInstance();
$group = array(
'id' => array('sanitizerType' => 'NULL_ACTION', 'method' => 'both', 'pages' => array('edit_orders'), 'params' => array()));
$sanitizer->addComplexSanitization($group);
$group = array(
'update_products' => array('sanitizerType' => 'NULL_ACTION', 'method' => 'both', 'pages' => array('edit_orders'), 'params' => array()));
$sanitizer->addComplexSanitization($group);
Now of course, those should only be considered temporary fixes as they basically ignore sanitization for id and update_products, whereas what should be added is
a MULTI_DIMENSIONAL sanitizer
I'll pull your most recent updates down for my test setup. It seems like the product's name and attribute name/value pairs will all need the NULL_ACTION sanitization -- is that correct?
Should there be a general sanitizer group for floats? The qty value that is included in the EO POST variables is a floating-point value, not an int.
Here's what I've come up with (so far) for the EO sanitizer; please let me know if there's a better way. This approach gets the product's name and text attributes to not get &'d to death:
Code:$eo_sanitizer = AdminRequestSanitizer::getInstance();
$eo_group = array(
'update_products' => array(
'sanitizerType' => 'MULTI_DIMENSIONAL',
'method' => 'post',
'pages' => array('edit_orders'),
'params' => array(
'update_products' => array('sanitizerType' => 'CONVERT_INT'),
'qty' => array('sanitizerType' => 'CONVERT_INT'), //-This one should really be a float
'name' => array('sanitizerType' => 'PRODUCT_DESC_REGEX'),
'onetime_charges' => array('sanitizerType' => 'CURRENCY_VALUE_REGEX'),
'attr' => array(
'sanitizerType' => 'MULTI_DIMENSIONAL',
'params' => array(
'attr' => array('sanitizerType' => 'CONVERT_INT'),
'value' => array('sanitizerType' => 'PRODUCT_DESC_REGEX'),
'type' => array('sanitizerType' => 'CONVERT_INT')
)
),
'model' => array('sanitizerType' => 'WORDS_AND_SYMBOLS_REGEX'),
'tax' => array('sanitizerType' => 'CURRENCY_VALUE_REGEX'),
'final_price' => array('sanitizerType' => 'CURRENCY_VALUE_REGEX'),
)
)
);
$eo_sanitizer->addComplexSanitization ($eo_group);
Will add this :)Quote:
Originally Posted by lat9
So I've pushed up a change to my testing branch to add a FLOAT_VALUE_REGEXQuote:
Originally Posted by lat9
I guess that probably 'tax' and 'final_price' should strictly speaking be floats.
In fact I think the CURRENCY_VALUE_REGEX may be unnecessary.
I'm not quite ready to push my testing branch to a full blown PR against core code as I want to make sure the unit tests cover some edge cases first.
Just checking in to see where the Zen Cart base code is on this issue. So far, my tests with the eo_sanitizer supplied above and the April 7 version of the Zen Cart changes looks sound.
One request: When Zen Cart is updated with these sanitizer changes, pretty-please make it a formal release for either Zen Cart 1.5.5a or 1.5.6 so that EO can determine the environment in which it's loading/installing and guide the installer to the proper version if plain-old-original Zen Cart 1.5.5 is currently being used.
HiQuote:
Originally Posted by lat9
I just pushed some final changes to the code, this shouldn't affect anything you have been testing as it mainly revolves around custom sanitizers and unit testing.
Will do the PR against core tonight.
Your comment regarding release numbering is noted and this will be what happens.
@wilt, I just found the following warning in my /logs folder; it's issued when the edit_orders script is initially entered (i.e. no $_POST parameters).
Here's the current code, with line 511 highlighted:Code:PHP Warning: Invalid argument supplied for foreach() in C:\xampp\htdocs\testsite\testadmin\includes\classes\AdminRequestSanitizer.php on line 511
It looks like you'd want to change that toCode:private function filterMultiDimensional($parameterName, $parameterDefinition)
{
$requestPost = $_POST;
foreach ($requestPost[$parameterName] as $key => $value) {
$hacked = $requestPost[$parameterName][$key];
if (isset($parameterDefinition['params'][$parameterName])) {
unset($requestPost[$parameterName][$key]);
unset($_POST);
$_POST[$parameterName] = $key;
$type = $parameterDefinition['params'][$parameterName]['sanitizerType'];
$params = isset($parameterDefinition['params'][$parameterName]['params']) ? $parameterDefinition['params'][$parameterName]['params'] : null;
$newParameterDefinition = array('sanitizerType' => $type, 'params' => $params);
$this->runSpecificSanitizer($parameterName, $newParameterDefinition);
$newKey = $_POST[$parameterName];
$requestPost[$parameterName][$newKey] = $hacked;
}
foreach ($hacked as $pkey => $pvalue) {
if (isset($parameterDefinition['params'][$pkey])) {
unset($requestPost[$parameterName][$newKey][$pkey]);
unset($_POST);
$_POST[$pkey] = $pvalue;
$type = $parameterDefinition['params'][$pkey]['sanitizerType'];
$params = isset($parameterDefinition['params'][$pkey]['params']) ? $parameterDefinition['params'][$pkey]['params'] : null;
$newParameterDefinition = array('sanitizerType' => $type, 'params' => $params);
$this->runSpecificSanitizer($pkey, $newParameterDefinition);
$requestPost[$parameterName][$newKey][$pkey] = $_POST[$pkey];
}
}
}
$_POST = $requestPost;
}
Code:private function filterMultiDimensional($parameterName, $parameterDefinition)
{
$requestPost = $_POST;
if (isset ($requestPost[$parameterName])) {
foreach ($requestPost[$parameterName] as $key => $value) {
$hacked = $requestPost[$parameterName][$key];
if (isset($parameterDefinition['params'][$parameterName])) {
unset($requestPost[$parameterName][$key]);
unset($_POST);
$_POST[$parameterName] = $key;
$type = $parameterDefinition['params'][$parameterName]['sanitizerType'];
$params = isset($parameterDefinition['params'][$parameterName]['params']) ? $parameterDefinition['params'][$parameterName]['params'] : null;
$newParameterDefinition = array('sanitizerType' => $type, 'params' => $params);
$this->runSpecificSanitizer($parameterName, $newParameterDefinition);
$newKey = $_POST[$parameterName];
$requestPost[$parameterName][$newKey] = $hacked;
}
foreach ($hacked as $pkey => $pvalue) {
if (isset($parameterDefinition['params'][$pkey])) {
unset($requestPost[$parameterName][$newKey][$pkey]);
unset($_POST);
$_POST[$pkey] = $pvalue;
$type = $parameterDefinition['params'][$pkey]['sanitizerType'];
$params = isset($parameterDefinition['params'][$pkey]['params']) ? $parameterDefinition['params'][$pkey]['params'] : null;
$newParameterDefinition = array('sanitizerType' => $type, 'params' => $params);
$this->runSpecificSanitizer($pkey, $newParameterDefinition);
$requestPost[$parameterName][$newKey][$pkey] = $_POST[$pkey];
}
}
}
}
$_POST = $requestPost;
}
Fix pushed hereQuote:
@wilt, I just found the following warning in my /logs folder; it's issued when the edit_orders script is initially entered (i.e. no $_POST parameters).
https://github.com/zencart/zencart/p...16edf14a41984d
It turns out that the add-product handling from Edit Orders has some challenges fitting into the mold provided by the sanitizer, when attributes are involved.
If you look, for instance, at the demo product (Big Linked/Bug's Life ...) cPath=22&products_id=34. That product has multiple attributes, including one (the Gift Options checkboxes) that presents itself as an array. Here's a dump of the $_POST information coming back from an add of that product (pre-sanitization):
So, the 'value' field of that attributes' list can be either a number, a collection of characters or an array! I'm not sure how to encode that currently; any guidance would be appreciated.Code:Array
(
[securityToken] => 608c5b6efd7f704accd12713fa833510
[id] => Array
(
[1] => Array
(
[value] => 85
[type] => 2
)
[2] => Array
(
[value] => 42
[type] => 0
)
[5] => Array
(
[value] => 48
[type] => 0
)
[6] => Array
(
[value] => 45
[type] => 0
)
[13] => Array
(
[value] => Array
(
[63] => 63
)
[type] => 3
)
[10] => Array
(
[value] => Here's a line of "text"
[type] => 1
)
[9] => Array
(
[value] =>
[type] => 1
)
[11] => Array
(
[value] =>
[type] => 1
)
)
[add_product_categories_id] => 22
[add_product_products_id] => 34
[search] =>
[step] => 4
)
I believe I've found the way to "describe" those inputs to the sanitizer, adding the following to the Edit Orders sanitizer:
@wilt or @DrByte, would you review/comment as to whether the PRODUCT_DESC_REGEX is appropriate for this structure?Code:'id' => array (
'sanitizerType' => 'MULTI_DIMENSIONAL',
'method' => 'post',
'pages' => array ('edit_orders'),
'params' => array (
'id' => array ('sanitizerType' => 'CONVERT_INT'),
'type' => array ('sanitizerType' => 'CONVERT_INT'),
'value' => array ('sanitizerType' => 'PRODUCT_DESC_REGEX'),
),
)
Hi,
grrrr.
I guess as a quick fix you could assign a NULL_ACTION to the value parameter as part of a MULTI_DIMENSIONAL sanitizer.
Fortunately you caught me at a point where I was preparing a PR for the sanitizer due to a different regression error.
It would be nice to have a generic sanitizer for this case, and I may work on that, but for now I will probably just look at doing an ATTRIBUTES_VALUE sanitizer.
Watch this space :)
Quote:
Originally Posted by lat9
Hi
That does in fact seem like a cool solution, although DrByte and I were talking and think PRODUCT_NAME_DEEP_REGEX is better than PRODUCT_DESC_REGEX as it is less permissive.
Quote:
Originally Posted by lat9
HI
On the other hand we need to consider what the TEXT attribute was meant for.
Assume you have a T-Shirt shop where the customer can define the text that appears on the T-Shirt
Using PRODUCT_NAME_DEEP_REGEX means the while the customer could order a t-shirt with
<script>Some Message</script>
using
PRODUCT_NAME_DEEP_REGEX as a sanitizer , would mean edit_orders would reject that,
So maybe PRODUCT_DESC_REGEX is a better option
Quote:
Originally Posted by wilt
As I posted in the EO forum, that construct I posted worked with pre-ZC1.5.5a postings of the AdminSanitizer class, but fails with the ZC1.5.5a version.
I finally got the AdminRequestSanitizer to spit out some debug (shouldn't it have been as simple as calling the setDebug function with an argument of true?). It looks like part of the issue is that EO uses the variable named "id" (just like the storefront) to hold those added-products' attributes and the built-in 'id' definition is taking precedence:
Note: I don't think it's a possibility to change that 'id' variable's name; there might be other non-EO code that's making use of that variable and I don't want to create a real train-wreck.Code:May-13-2016 11:51:00
=================================
Incoming GET Request Array
(
[page] => 1
[oID] => 8421
[action] => add_prdct
)
Incoming POST Request Array
(
[securityToken] => cb8ec48cbfe04dbde8aeb3dd0a4f7b34
[id] => Array
(
[35] => Array
(
[value] => 2271
[type] => 0
)
[48] => Array
(
[value] => 3244
[type] => 0
)
[43] => Array
(
[value] => 2406
[type] => 0
)
[20] => Array
(
[value] => 2259
[type] => 0
)
[44] => Array
(
[value] =>
[type] => 1
)
)
[add_product_categories_id] => 110
[add_product_products_id] => 287
[search] =>
[step] => 4
)
Running Admin Sanitizers
PROCESSING SIMPLE_ALPHANUM_PLUS(GET) == action
PROCESSING SIMPLE_ALPHANUM_PLUS(POST) == id
PROCESSING SIMPLE_ALPHANUM_PLUS(GET) == oID
PROCESSING SIMPLE_ALPHANUM_PLUS(GET) == page
PROCESSING STRICT_SANITIZE_VALUES == securityToken
PROCESSING STRICT_SANITIZE_VALUES == add_product_categories_id
PROCESSING STRICT_SANITIZE_VALUES == add_product_products_id
PROCESSING STRICT_SANITIZE_VALUES == search
PROCESSING STRICT_SANITIZE_VALUES == step
Outgoing GET Request Array
(
[page] => 1
[oID] => 8421
[action] => add_prdct
)
Outgoing POST Request Array
(
[securityToken] => cb8ec48cbfe04dbde8aeb3dd0a4f7b34
[id] => Array
(
[35] => Array
[48] => Array
[43] => Array
[20] => Array
[44] => Array
)
[add_product_categories_id] => 110
[add_product_products_id] => 287
[search] =>
[step] => 4
)
FWIW, editing /admin/includes/init_includes/init_sanitize.php and commenting out the "built-in" processing for the id parameter (lines 81 and 172) and using the construct that I posted above appears to produce the desired results.
Not knowing why the id parameter was added to the built-in list (twice, so it must be very important:P), I consider that change a work-around rather than a proper correction.
I'll take a work-around! :) OK will test tonight and let you know. Can you push to your repo?
This should work.
I note that if I add $sanitizer->setDebug(true); to the end of admin/includes/init_includes/init_sanitize.php I get Sanitize logs
Quote:
Originally Posted by lat9
I've pushed the update to the eo_sanitization.php file to account for the add-product path (including the corrected check). I have not (and won't unless it's absolutely needed) pushed the work-around.Quote:
Originally Posted by swguy
When I added $eo_sanitizer->setDebug(true); to the end of admin/includes/extra_datafiles/eo_sanitization.php ... it doesn't.Quote:
Originally Posted by wilt
The work around (the change to admin/includes/init_includes/init_sanitize.php suggested in https://www.zen-cart.com/showthread....78#post1310878) appears to not be needed. Your change to admin/includes/extra_datafiles/eo_sanitization.php works perfectly for the issues I was having.Quote:
Originally Posted by lat9
Thanks for your hard work on this fix.
swguy, you're right!:clap: When I remove those commented-out lines from my client's fresh 1.5.5a/EO4.1.5-beta2 installation ... adding a product with attributes works! Sounds like it might be time to package up EO v4.1.5 for release (more likely tomorrow).Quote:
Originally Posted by swguy
Cool. Great news.