how is this beta addon idea?

I've just completed the main functional part of a nicely working filter. A simple nicely working filter and I've taken care of the securities of the url parameters. I'll be putting together some js to go with it and submitting it as an addon soon. Is there a way to get a copy of the files up here for beta testing??

Thanks, John

I'm a bit concerned about the security of the price and manufacturer filters in this code in the index_filters area, since that code runs on page load, and not after being filtered thru the product_filter_sidebox module:```
if(isset($_GET['jpricelohi']) && zen_not_null($GET['jpricelohi'])){
$loHiParts = explode('',$_GET['jpricelohi']);
if(isset($loHiParts[0]) && zen_not_null($loHiParts[0])){
$and .= " and p.products_price >= ".$loHiParts[0];
}
if(isset($loHiParts[1]) && zen_not_null($loHiParts[1])){
$and .= " and p.products_price <= ".$loHiParts[1];
}
}
if(isset($cleanJpManus) && zen_not_null($cleanJpManus)){
$manu_id_string = '';
foreach($cleanJpManus as $manu_arr_flt){

        $manu_id_string .= $manu_arr_flt.',';
        }
        $manu_id_string = rtrim($manu_id_string, ',');
        $and .= " and m.manufacturers_id IN (".$manu_id_string . ")";
    }

Also, the /includes/index_filters folder does support overrides.

So you can move your custom one into /includes/index_filters/YOURTEMPLATE/default_filter.php

But you should document in the readme that the music_filter and record_company_filter aren't supported by your plugin. (And that's probably not a problem unless someone filtering those products is actually doing a different kind of filtering than those are already triggering.)

You'll also want to document in your readme that your plugin expects the database to contain 'products_short_desc' and 'products_special_data' fields in the products_description table. Or remove those from the queries you've altered in the plugin.

didn't realize I could override the default_filter.php. Awesome!

As for the security of the price sorter, I'm tapping into the built-in ZC 'sort' so it's cleansed by application_top.php. Isn't this correct?

I'll be removing the product_short_desc. Didn't realize that was in there.

Thanks for the feed back!

Ahhh, I see. You're talking about the price filter and not the price sorter.... any recommendations on this?

So I've moved the security testing into the default filter. It seems as though I've not completely nailed the security though. Please come back with a better method of cleansing .

Thank You.

for price, maybe this:

$loHiParts = explode('_', preg_replace('/[^0-9_]/', '', $_GET['jpricelohi']));

That removes anything that's not numeric or underscore.

Is 'jparam_manu' supposed to be an array? If so then you'll have to loop thru all the array values and sanitize them before adding them to the query.

Is it necessary for all of these to be $_GET? It's best to limit the use of $_GET because it clutters the URL, and then it makes things complicated for setting canonical stuff for Google to understand what are "duplicate content" pages vs unique pages.
I know the advanced-search uses $_GET, but if this filter can be done via POST it'd be better.
Using $_GET will cause the URL to have a bunch of ugly content like:

&jparam_manu[]=foo&jparam_manu[]=bar&jparam_manu=1&jparam_manu=2&jparam_manu=3&jpricelohi=5_56789

And maybe something this for manufacturers?

        $manu_arr_flt = array();
        foreach($_GET as $key => $value){
            if(preg_match('/^jparam_manu/', $key)){
                $manu = preg_replace('/[^0-9]/', '', $value);
                if ($manu != '')
                {
                    $manu_arr_flt[] = $manu;
                }
            }
        }
        if (sizeof($manu_arr_flt)) {
            $and .= " and m.manufacturers_id IN (". implode(',', $manu_arr_flt) . ")";
        }        

(These are all for the default_filters.php file)

When you're ready to package, I recommend removing any trailing ?> from the end of all PHP files. This prevents stray "blank lines" from getting treated as HTML when certain FTP programs mangle the line-endings in the files during transfer.
Ref: http://www.zen-cart.com/content.php?271-some-of-my-php-files-are-missing-the-tag-at-the-end-of-the-file

okay so I've changed to the following, but I don't understand why dropping a ' # ' or a ' % ' into one of the $_GETs still breaks stuff. Isn't zen checking for this?

if(isset($_GET)){
    $cleanJpManus =  array();
    foreach($_GET as $key => $value){
        if(preg_match('/^jparam_manu/i', $key)){
        $clean = preg_replace('/[^0-9_]/', '', $value);
            $cleanJpManus[] = $clean;
        }
        
    }
    if(sizeof($cleanJpManus) == 0){unset($cleanJpManus);}
}

if(isset($_GET['jpricelohi'])){
    $cleanJpLoHi = preg_replace('/[^0-9_]/', '', $_GET['jpricelohi']);
}

Both of those characters in the uri string itself have their own definition. They would need to be escaped in some manner if they are to mean something other than their original definition. See RFC 3986 for some more information about uri characters. Yet also one more reason not to pass the data via GET: http://www.ietf.org/rfc/rfc3986.txt

no, I don't want them there. If they're present ( hack attempt ) they need to be removed. The regex/preg_replace does work to remove the chars but apparently not before zen breaks....

bumba000:

okay so I've changed to the following, but I don't understand why dropping a ' # ' or a ' % ' into one of the $_GETs still breaks stuff.
What exactly do you mean by "breaks stuff"? Some sort of error message perhaps? Without knowing what's breaking and where, it's kinda tough to tell you what to change.

bumba000:

Isn't zen checking for this?Not for parameters it doesn't know about, no. There are some legitimate other scenarios where those characters are valid. So, since you're adding completely new functionality, it's up to you to sanitize your own stuff.

BTW, in a regex, the /i means "case insensitive", but everything you're doing doesn't need to be case-insensitive, so you can leave off the "i" and make the parser run faster.

ok. I moved the sanitize stuff into functions/safety_functions.php and overridden it. The trouble is that

$_GET['jpricelohi'] = preg_replace('/[^_0-9]/', '', $_GET['jpricelohi']);

doesn't remove ( % # * ) and so on and it breaks stuff, which we know isn't a good sign. I can not seem to find a regex combination that works and leaves 0-9 and _ . What the heck?!

I'm still working on it though. If you have a solution please, do tell.

Thank You, John

bumba000:

ok. I moved the sanitize stuff into functions/safety_functions.php and overridden it.
That's not really necessary.
Simply sanitizing the inputs where you use them is preferable.

bumba000:

and it breaks stuffAgain, you've not yet explained what you mean by "breaks stuff".

The code $foo = preg_replace('/[^0-9_]/', '', $foo) does remove anything that's not 0-9 or underscore. I'm guessing you're testing that in the wrong place if you're getting unexpected results.

Handy: http://regexpal.com/

uhhh, safety_functions.php is from another site, completely custom. So, sorry bout that!

The file is init_includes/overrides/init_sanitize.php. This is where I have the preg_replace...

Now. Ok, so load up the site. hit the price range filter (not sure why i called it loHi) anyways, changing the data in the url parameter to something like jpricelohi=96_1%89 or jpricelohi=96_1#89 after the preg_replace has run on $_GET['jpricelohi'] will either whitescreen with warning: refresh... or the hashtag makes it through but stops the functionality of the module and an echo of $_GET['jpricelohi'] then says: 96_1 .

If I do an all in php test like

$dirty = '99_434%34';
$dirty = preg_replace('/[^_0-9]/', '', $dirty);  
echo $dirty;

I get ( 99_43434 )

if I do the exact same preg_replace on the url parameter $_GET['jpricelohi'] I can't tell if it works or not because if I put an % symbol into the url like ( 99_434%34 ) , I get a "page isn't redirecting properly". Replace the % with the $ symbol and I get "WARNING: An Error occurred, please refresh the page and try again."

But any of these funky characters in the $_GET['sort'] seem to be cleaned right up and the system moves along.

I have moved the preg_replace's to where the code is being executed on (as suggested). Still the same though when entering unwanted chars into the URL.

The feedback I've given so far is from inspecting the code.

I just installed it to test on a demo site, and the sidebox doesn't even display, despite being enabled in the admin. Tried on home page, category page, product page, pretty much everywhere.
So, I hacked $show_product_filter_sidebox = true; and got it to display, but then it only displays Lo-to-Hi and Hi-to-Lo choices. Nothing about prices or manufacturers.

So I checked, and this line: ```
if(!$listing->EOF && $_GET['main_page'] == 'index'){

Given that this is seemingly intended to simply check whether there are any listing results shown, there are some other options:
- if ($listing->RecordCount() > 0 && $current_page_base == 'index') {
- if ($listing_split->number_of_rows > 0 && $current_page_base == 'index') {
I'm inclined to use the latter.


I also discovered I'd made a couple typos in my earlier code suggestions: I used "explode" instead of "implode" in the manufacturer SQL, and forgot a closing parenthesis. I've corrected the post above. 
This would lead to db errors (the "WARNING" you mentioned) if you're testing any manufacturer option at all.

The % is an odd duck, since using %34 gets treated as a single character, and thus has the effect of potentially invalidating the next parameters you've passed. 

Also, any time I click on the Filter submit button it just takes me to the home page. I'm assuming that's because there's no <form> "action" specified in the sidebox template?

That's surprising that the box doesn't show for you. Im on 1.5.1 (if it makes any difference). The idea is that the box will only display on a page which uses $listing and is not search, new, specials...

I tested the two conditions you've supplied and neither let the box show
if ($listing->RecordCount() > 0 && $current_page_base == 'index')
if ($listing_split->number_of_rows > 0 && $current_page_base == 'index')

I did notice that you had left off that closing round but completely overlooked the explode vs implode!

Wonder why we're having two way different experiences with this incredibly simple plugin. Really it's just the 1.3.8 blank_sidebox with a modification to a single core file (default_filter.php which is now overridden).

I've been trying to figure out where the mistake was made now with that implode / explode, you edited the post!! Funny. Well it works now.

Any luck getting the mod to work on your dev site?

Also, the form submit url being blank obviously has been working for me. Perhaps it's browser related...
I've fixed it though.

in includes/templates/YOTEMP/sideboxes/tpl_product_filter_sidebox.php
add the lines

$uri_parts = explode('?', $_SERVER['REQUEST_URI'], 2);
$actionURL = $uri_parts[0];

and of course, change the line

  $content .= zen_draw_form('cust_product_filter', '', $method = 'get', $parameters = '');

to

  $content .= zen_draw_form('cust_product_filter', $actionURL, $method = 'get', $parameters = '');

No problems.