Lack of Server-Side Price Data Type Checking Results in Wrong Price

[zschuessler]

Issue

Price data type is not checked in multiple locations. This can sometimes result in a bug where price is a string. In this scenario, a string that contains a comma will cause the PHP compiler to incorrectly cast the string to a number or float value.

Reproduce

Specific example with this function:
https://github.com/woothemes/woocommerce/blob/c6dae2b2866dfd42db2b092a426d01d1f3b49892/includes/abstracts/abstract-wc-product.php#L798

This line specifically:

$price = $price * $qty;

Lack of type checking will result in the following pricing error:

$price = '2,000';
$qty = 5;
$price = $price * $qty; // results in '10'

Solution

This issue is mitigated in the admin UI by not allowing commas. However, custom imports and custom pricing-related code could fall victim to this. Data type checking/casting would be ideal to provide further integrity in pricing functions.

Note that the "Quick Edit" feature on the Product list page in the admin UI also does not have validation, so it's another point of failure without type checking.

[mikejolley]

Type cannot be checked; post meta (where prices are stored) stores everything as strings, not floats etc. So when you can a string out, you cannot really determine if its valid or not. Regarding quick edit (and all other admin areas for setting price) you'll note:

$new_regular_price = $_REQUEST['_regular_price'] === '' ? '' : wc_format_decimal( $_REQUEST['_regular_price'] ); update_post_meta( $post_id, '_regular_price', $new_regular_price );

wc_format_decimal is used. This removes any locale specific decimals, such as comma, and replaces with '.' to keep prices consistent when stored as strings.

https://github.com/woothemes/woocommerce/blob/5920b88d5db635d07af065afcbf0c6f0a66c2b9e/includes/wc-formatting-functions.php#L185

[zschuessler]

Thanks for the explicit writeup Mike.

I think there was a slight bit of confusion, though. The issue is that having the comma in the specific code I linked to - it will cause a pricing bug. It sounds like wc_format_decimal should be used there to remove the comma (which causes the bug). I don't think it's a solution to rely solely on sanitizing input in the database. A few reasons why:

1. You noted that the sanitize function is run on the Quick Edit save handler. Something is awry - I'm able to put in "2,000.00" as a price on the Quick Edit screen, and on the product detail frontend I see "2" as a price. I can file that as a separate bug with info on my setup if you can't duplicate.
2. Custom import code will inevitably cause developers hardship if their data contains commas.
3. Custom third-party plugin/extended code cause issues similar to point 2.

Ultimately this causes technical debt that can be avoided.

[mikejolley]

@zschuessler The problem is only slightly mitigated if we sanitise price on retrieval via product class. If you're access that through post meta it still may be malformed, which again is an issue for plugins.

I personally feel it should be formatted on input only.

BTW, the reason commas may be saving for you is that your locale is setup to use real decimals, in which case , may not be removed. Is this the case?