[Not a bug] Preventative action: bug to be introdiced in 1.3.9/2.0.0

[conor]

Hi,

I was recently made aware of the following page:

https://www.zen-cart.com/tutorials/i...hp?article=320

The following lines cannot be made part of the core or they will break many payment modules:

if (isset($_GET[$key]) && strlen($_GET[$key]) > 43) {
    $contaminated = true;
  } 

This is because they are limiting the length of any single GET parameter to 43 characters.

Many of the payment modules pass an error message as a GET parameter which is considerably longer than 43 characters - something which is fully compliant with all relevant standards/guidelines.

Either the error_message parameter should be removed from the $paramsToCheck array or the allowed length of the parameters should conform to the web standards for GET parameters.

Obviously it would be a critical mistake to implement this code as it is in 1.3.9/2.0.0, so I hope that the above comments will be taken onboard and that Zen Cart won't break standards for overzealous "peace of mind" issues! ;)

All the best...

Conor

[DrByte]

Conor,

You are correct that a problem would exist if the $_GET method were to continue to be used for error message handling from payment modules.
However, the correct way to handle error messages that are passed from page to page is to do it via the messageStack and not via a $_GET parameter.

Revising the payment module to use the messageStack will avoid the problem you mention.

[conor]

Hi,

However, the correct way to handle error messages that are passed from page to page is to do it via the messageStack and not via a $_GET parameter.

Revising the payment module to use the messageStack will avoid the problem you mention.

Either method was a "correct" way before, but I'll have to rewrite things to dump the "old" usage of the error_message parameter. That's a lot of extra work for me.

I'll set aside a few hours for this then. :|

Are there any documents yet about what changes will be needed for modules for 1.3.9/2.0.0?

Conor

[conor]

Hi,

Since payment modules are intended to use the messageStack functionality from Zen Cart 1.3.9 onwards I think the following lines in checkout_payment/header_php.php will be defunct from 1.3.9 onwards and should be removed:

if (isset($_GET['payment_error']) && is_object(${$_GET['payment_error']}) && ($error = ${$_GET['payment_error']}->get_error())) {
  $messageStack->add('checkout_payment', $error['error'], 'error');
} 

The maximum length of any error message would be 43 characters in the PayPalWPP module, and for any third party modules it would seem pointless to manage the message passing using session variables when messageStack's add_session method could just be used instead, before redirecting back to the payment page.

For these reasons I recommend that you remove support for get_error method in payment modules from 1.3.9 onwards.

This will ensure that other authors of payment modules will also update their error handling code and won't fall foul of the soon-to-be-introduced "contamination" functionality.

Hope that helps!

All the best...

Conor

[conor]

bump!!!

[DrByte]

"bump"?

Your suggestion is understandable and will be considered.

[conor]

Hi,

"bump"?

Your suggestion is understandable and will be considered.

lol that was a bit cheeky, wasn't it? :)

I just wasn't sure if anyone had read my suggestion. Thanks for getting back to me. It's not exactly a major issue but I thought it would be better to clarify things by having a single method for passing error messages.

I've started converting the payment modules I've written over to using the messageStack system and it's taking less time than I originally thought, and works well, which is good! :)

Thanks again.. I suppose you can consider this thread "finished"!

All the best...

Conor
ceon

[DrByte]

Yes, it was cheeky. Sorry.

Deprecating that messageStack output in the template in the v1.3.x series is probably not a good idea for compatibility reasons. Granted, the sanitizer is going to kill the page if the longer message is sent. It's a bit of a challenge, for which there is no perfect answer for everyone.

As for 2.0, payment modules will have to use the messageStack for error messages.