httpredir url parameters may not be properly encoded

[briansandall]

For example, searching for '%bag' converts to '%BAg' in the URL; this is a special character that ends up dropping the entire search string from the query (according to the debug sanitize GET log) and returns no results.

Searching for '%210' turns into '!0', but remains as a valid query, though clearly without the intended products showing up.

Notably, the AJAX search results that appear while typing work properly - it is only after submitting the form that the query is sent as is rather than being urlencoded.

[briansandall]

Search terms are passed from products.index.inc.php via $_POST['search']['product'].

Debugging from functions.inc.php::httpredir:

No prior urlencode

Original $destination: ?_g=products&q=%bag
URLDecoded: ?_g=products&q=�g

URL encoded before sending to httpredir

Original $destination: ?_g=products&q=%25bag
URLDecoded: ?_g=products&q=%bag

Since the '%' symbol is still not encoded, this particular search query stilll gets dropped from the GET parameters during sanitizing, preventing a proper search result.

Typing the parameter into the URL directly as '%bag' gets dropped; as '%25bag' returns results as expected.

The only way to resolve this that I can think of is to break up the url in httpredir, explode the query parameters and rebuild it, calling urlencode on each parameter value.

[briansandall]

I had initially handled anchors but saw that it was already taken care of, so removed that.

Note that urlencoding $_POST['previous-tab'] completely breaks the tabs, so I removed that as well.

[bhsmither]

This seems to be searching from the flyout search box in admin?

The same happens when searching for %210 from the storefront Advanced Search page, but not from the Quick Search text entry field. This is because the Advanced Search POST parameters gets rebuilt to a querystring, then sends it gets sent through httpredir.

[bhsmither]

There will probably be similar situations with search terms having encoded #, ?, &, /, =, and brackets.

Shortly in to the httpredir() function, the $destination is urldecoded and html_entity_decoded. It is not re-coded.

[briansandall]

This issue affects any redirect that includes %21 or any other term with a special URL meaning.

The fix as suggested should handle any such characters by re-encoding them.

The quick search works because it is not done via httpredir(), but by AJAX.

[bhsmither]

The Quick Search starts at the point after which the values POSTed from the Advanced Search gets converted to a querystring and that the querystring is then submitted by the browser via the httpredir() browser bounce (not noticed by the visitor).

Your observation may be that your Quick Search text entry field has been replaced by a "Suggestive Search" extension using AJAX. (I am saying that the Quick Search does not use AJAX.)

[briansandall]

By 'Quick Search' I assumed you meant the search results that appear when typing in the admin search box that pops up.

I'll take a look at the customer side search. Interesting, that search field gets encoded correctly. Tested with my PR and the Quick Search continues to function correctly, as do the rest of the searches.

[briansandall]

I believe the reason the Quick Search works correctly is because it uses the $_GET protocol when submitted, so any form data should be automatically encoded properly for use in a URL.

Note that both the advanced search and admin searches use $_POST

[bhsmither]

To clarify, my comments are focused on the storefront. But we can easily find a use-case in admin. For example, Product Reviews, Search tab, uses httpredir() to have the browser bounce to using a querystring from previously POSTed parameters.

Also, to clarify, the browser properly encodes troublesome characters both in POST and the querystring.

It is not even in the POST to querystring conversion in the Catalogue class where the corruption occurs.

It is when CubeCart wants the browser to resubmit and uses httpredir() to do it.

[briansandall]

Have you checked out my PR? Because it addresses that exactly. It's not a question of use case, it's a fact that httpredir as coded does not properly enforce encoding.

What are you worried about for the storefront? I'm not understanding the context of your comments.

[briansandall]

Though it's still not possible to search ? and & - anything after and including these characters gets dropped. So if you search 'M&M Candy', it ends up trying to search 'M' instead. This will need to be fixed.

Another interesting note - there is a different 'no result' page for different characters when included in a search:

=
The following errors were detected:
No matching products found. Please try again.

Vs:
/, #
Products found matching 'search/' (note that the search term is displayed)
No products found.

I haven't looked into why that occurs, but it does.

[briansandall]

Fixed = returning the weird error page by limiting the split on = to 2 strings:

$param = explode('=', $part); // key=some=thing would return 3 strings
$param = explode('=', $part, 2); // this restricts it to 'key' and 'some=thing'

EDIT: Did the same for ? and that now works properly. All that remains is &.

[briansandall]

Note that searching 'M&M Candy' via the Quick Search also fails despite being properly encoded, giving the 'error' version of no matching results.

This leads me to suspect that there may be a separate issue, although the advanced and other searches still end up dropping '&M Candy', whereas the Quick Search leaves the entire search term intact.

[briansandall]

Latest commit fixes issues with & inside a query parameter when using httpredir, but the Quick Search still fails despite having what appears to be the same URL.

// Advanced search URL (succeeds or fails with standard 'no products found' page):
http://localhost/git/forks/cubecart/v6/search.html?_a=category
    &search%5Bkeywords%5D=M%26M+Candy
    &search%5BpriceMin%5D=&search%5BpriceMax%5D=&sort%5BRelevance%5D=ASC

// Quick Search URL (fails with 'error' search results):
http://localhost/git/forks/cubecart/v6/search.html
    ?search%5Bkeywords%5D=M%26M+Candy
    &_a=category

[briansandall]

Even more interesting is that sorting by anything (including nothing) other than 'Relevance' returns the error 'no results' page.

This seems to be the cause of the Quick Search failing as well, since the URL is properly encoded and the search term is being searched. Will look into the ORDER BY section.

[briansandall]

Further investigation shows that the 'error' notification only occurs when no products are found, per cubecart.class.php::919.

After adding an 'M&M' product for testing, the search worked perfectly (Quick Search both before and after this fix, Advanced Search and others only after this fix).

I'm not sure why there would be different error pages, but I believe that can be addressed in a separate issue.

Unless anyone can think of other issues with the current PR, I believe it fixes what I set out to fix.

[bhsmither]

"Have you checked out my PR?"

I have scanned over it, but I haven't black-boxed it yet. (Black-box: something unknown, but has an input and an output. Feed it all manner of input, rational and irrational, and see what comes out in an effort to determine what it does.)

[briansandall]

Fair enough - I've put a fair number of different inputs through, but there always seems to be an edge case. It's times like these when a good set of unit tests would come in handy... :P

Another important thing to test is the standard redirects that occur during the course of admin / customer use. I haven't yet tried using a payment gateway, for example, though I've done a fair amount of work in the admin panel and everything seemed fine.

[bhsmither]

For example, a search term:
apple================pie
Again, not having black-boxed the PR, I think I can always come up with a term (as idiotic as an idiotic visitor to the site can come up with) that the PR cannot handle.

[briansandall]

The PR actually handles that term just fine (returning the 'error: no matching results' page as mentioned above - the searched term is correct, however).

I do agree that some dedicated testing would be beneficial.

[bhsmither]

Here's a test: create a product that does have apple=================pie as the product name or is in the description.

[briansandall]

It works just fine. You seem very concerned - perhaps you can run some tests yourself?

[Dirty-Butter]

I'm not pretending to say I understand all that's involved in this discussion, but I would say that Bsmither's created Search RLIKE code for our plushcatalog store DOES search properly for M&M in the store search box. It does NOT work in the admin search, nor the Advanced store search (searches for m).

[briansandall]

I did find one that didn't work: wild cards. E.g. 'Apple==%Pie' returns no results from Advanced Search, but does from Quick Search. Also works in the admin area, both before and after the suggested PR.

// Quick Search:
http://localhost/git/forks/cubecart/v6/search.html?search%5Bkeywords%5D=Apple%3D%3D%3D%25Pie&_a=category

// Advanced Search:
http://localhost/git/forks/cubecart/v6/search.html?_a=category&search%5Bkeywords%5D=Apple%3D%3D%3D%25Pie&search%5BpriceMin%5D=&search%5BpriceMax%5D=&sort%5BRelevance%5D=ASC

Note that the wild card search fails in Advanced Search prior to this PR as well. This appears to only affect sorts by 'Relevance'.

[briansandall]

@Dirty-Butter The regular search box has never had this problem - it is only a problem from Advanced Search or the admin area.

[bhsmither]

You seem very concerned

Concerned, yes, but not very concerned. Black-boxing is part of QA and I am very good at QA (except when I make my own mistakes). A devil's advocate is a very useful thing to have as it sometimes forces one to take a different approach to a solution.

[briansandall]

@bhsmither Yes, I agree. Your probing questions lead to several improvements in the PR already, which I appreciate. However, I don't have much more time I can spare on testing - the boss has other plans for me ;)

If you want to set up a black-box testing suite and run a bunch of tests, I'd be more than happy to review your results. Cheers!

[abrookbanks]

Fixed with pull request.