Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
85 stars 31 forks source link

Check if the order actually exists in wcs_order_contains_subscription() #344

Closed dennisnissle closed 1 year ago

dennisnissle commented 1 year ago

Describe the bug

The function wcs_order_contains_subscription() contains a helper which retrieves the actual order in case a numeric value (or any non-order-type) is handled by the function. The function should at least check whether the order does actually exist before accessing getters as the function is used, e.g. in the Woo Subscriptions core plugin which uses it like this in WC_Subscriptions_Payment_Gateways::get_available_payment_gateways( $available_gateways ):

if ( ! WC_Subscriptions_Cart::cart_contains_subscription() && ( ! isset( $_GET['order_id'] ) || ! wcs_order_contains_subscription( $_GET['order_id'] ) ) ) { return $available_gateways; }

This may lead to issues when the order does not exist.

haszari commented 1 year ago

Hi @dennisnissle - thanks for reporting this. Can you give us more information about what how this impacts your store?

For example, steps to trigger the error on a basic site with only WC core and WC Subscriptions active.

Thanks!

dennisnissle commented 1 year ago

Hi @haszari - Well, I guess the actual bug is obvious, isn't it? You cannot construct an order via wc_get_order() on unknown data and expect the actual order to exist. One needs to check whether the order actually exists or not before accessing internal getters of the actual order. This bug does not depend on third-party-extensions.

To reproduce:

  1. Enable Woo Subscriptions
  2. Go to WooCommerce > Settings > Payments
  3. Append &order_id=123 (the order ID should not exist) to the URL, e.g. my-site.com/wp-admin/admin.php?page=wc-settings&tab=checkout&order_id=123
  4. Fatal error is thrown
haszari commented 1 year ago

Thanks for explaining @dennisnissle !

Append &order_id=123 (the order ID should not exist) to the URL, e.g. my-site.com/wp-admin/admin.php?page=wc-settings&tab=checkout&order_id=123

So to be clear, how does this happen in practice for your store? Looks like this would be a relatively quick fix (feel free to submit a PR). At the same time, as I understand it the issue would be rare in practice, an edge case which doesn't impact merchants regularly.

dennisnissle commented 1 year ago

Hi,

in our case one of our customers reported the issue in combination with using Germanized for WooCommerce, so it seems like there are cases where the issue is relevant :) I've provided a PR for it.

Best, Dennis