AdminRequestSanitizer Error Log

[mc12345678]

Hi

I do have a pending commit that uses recursion.
https://github.com/zencart/zencart/pull/886/files

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?

I haven't been able to test related to this, but it has me curious...

[wilt]

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

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?

I haven't been able to test related to this, but it has me curious...

[wilt]

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.

This is not what is happening

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

[mc12345678]

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

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:

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'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. :)

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:

          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]);
          }
          }

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.

[wilt]

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.

[lat9]

@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?

[lat9]

@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.

I've got an updated version of EO on GitHub (https://github.com/lat9/edit_orders) that includes that work-around.

[wilt]

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

@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.

I've got an updated version of EO on GitHub (https://github.com/lat9/edit_orders) that includes that work-around.

[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 ...

[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

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

Since the id parameter is already sanitized, is there no hope?