Fix fatal error by checking for valid order before processing webhook

[FangedParakeet]

Fixes #4286

Changes proposed in this Pull Request

Fixing Fatal Errors From Fractured Webhooks

Please read the details within #4286 for more context on the need for this fix.

If the event body of the process_webhook_payment_intent_succeeded webhook event is not associated with a valid order ID for whatever reason, this fix will prevent a fatal error. All other functions within WC_Payments_Webhook_Processing_Service already have similar safety nets to handle mysteriously indecipherable orders and with these changes the process_webhook_payment_intent_succeeded handler will fall in line by throwing the same Invalid_Webhook_Data_Exception if an order is not to be found.

Testing instructions

1. Please pull the changes in this PR.
2. We will need to trigger the webhooks REST endpoint with an event type of payment_intent.succeeded. I'm not sure if there is a better way to do this, but I managed it by making a POST request to the endpoint using Postman. The target endpoint should be /wp-json/wc/v3/payments/webhook. Here is an example of a test request body. All of these fields will be required and the object id (here pi_123) must not match anything in your orders table.

{
  "id": "evt_123",
  "object": "event",
  "data": {
    "object": {
      "id": "pi_123",
      "object": "payment_intent",
      "charge": "ch_123",
      "status": "processing",
      "invoice": "inv_123",
      "metadata": [],
      "charges": {
          "data": [
              {
                  "id": "ch_123"
              }
          ]
      }
    }
  },
  "type": "payment_intent.succeeded"

3. If we make the above request without these changes, the response should be an internal_server_error that states that there has been a critical error on the website. However, with the changes we should now be greeted with a marginally friendlier 400 bad_request error instead.
4. Double-check that we threw the correct Exception by checking the WCPay status/error logs. There should be an error printed similar to the following: "ERROR WCPay\Exceptions\Invalid_Webhook_Data_Exception: Could not find order via charge ID: ch_123 in /var/www/html/wp-content/plugins/woocommerce-payments/includes/class-wc-payments-webhook-processing-service.php:365".

---

• Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
• Covered with tests (or have a good reason not to test in description ☝️)
• Tested on mobile (or does not apply)

Post merge

• Link to testing instructions from release testing doc following these instructions : Add link here / 'QA Testing Not Applicable'
• Add or update critical flows and testing instructions for critical flows, if applicable.

[vbelolapotkov]

Looking into code it looks like the bug is caused by this line, according to it missing order is valid case and execution should continue, rather than throw exception. However, this change stops execution and throws the exception.

@htdat looks like you were the author of get_order_from_event_body_intent_id (code), could you please clarify how the code is supposed to behave in fallback case.

Side note, this the second time when I see that the fatals are caused by mixed return type from the method.

[ismaeldcom]

I think the bug comes from this commit where $order is used without checking it.

get_order_from_event_body_intent_id was created here, but it just contains the code used in process_webhook_payment_intent_succeeded and was created to avoid duplicated code.

And as @vbelolapotkov mentions, we should not throw there, because that will prevent invoice.paid to run.

Probably the simpler approach is to just check $order before using it in process_webhook_payment_intent_succeeded. Although I don't know if that could bring any other issue. There are some new additions since I reviewed the failed payments PR.

[ismaeldcom]

Yes! process_webhook_payment_intent_succeeded was way shorter before, there were no additional uses of $order, but now we should wrap all of them within it or do an early return.

woocommerce-payments/includes/admin/class-wc-rest-payments-webhook-controller.php

Lines 287 to 294 in d619c4d

	private function process_webhook_payment_intent_succeeded( $event_body ) {
	$event_data = $this->read_rest_property( $event_body, 'data' );
	$event_object = $this->read_rest_property( $event_data, 'object' );
	$intent_id = $this->read_rest_property( $event_object, 'id' );
	$order = $this->get_order_from_event_body_intent_id( $event_body );
	WC_Payments_Utils::mark_payment_completed( $order, $intent_id );
	}

[zmaglica]

Side note, this the second time when I see that the fatals are caused by mixed return type from the method.

Either you return false/null or throw exception in this case. One of the problems is that I failed to see, as a reviewer, that this function can return multiple types. This should be covered with tests where every case that can occur during the execution of the function can occur.

[htdat]

I think the bug comes from this commit where $order is used without checking it.

Agreed this point!

get_order_from_event_body_intent_id was created here, but it just contains the code used in process_webhook_payment_intent_succeeded and was created to avoid duplicated code.

But looking further, the side effect coming from this commit as it introduces false for this condition branch:

woocommerce-payments/includes/admin/class-wc-rest-payments-webhook-controller.php

Lines 485 to 488 in d619c4d

	} elseif ( ! empty( $event_object['invoice'] ) ) {
	// If the payment intent contains an invoice it is a WCPay Subscription-related intent and will be handled by the `invoice.paid` event.
	return false;
	}

The original commit ref adding this condition means to return early and ignore this webhook:

woocommerce-payments/includes/admin/class-wc-rest-payments-webhook-controller.php

Lines 273 to 276 in 89766ab

	} elseif ( ! empty( $event_object['invoice'] ) ) {
	// If the payment intent contains an invoice it is a WCPay Subscription-related intent and will be handled by the `invoice.paid` event.
	return;
	}

[ismaeldcom]

But looking further, the side effect coming from this commit as it introduces false for this condition branch:

I prefer null over false for those cases. But in the end it will throw an error if we don't check if $order is valid, right? 🤔

[htdat]

@ismaeldcom - yes, returning null is ideal. But in this specific case, it just introduces another type for get_order_from_event_body_intent_id as in the body of this function, it's already having: $this->wcpay_db->order_from_intent_id() and $this->wcpay_db->order_from_order_id(), which are returning these types: boolean|WC_Order|WC_Order_Refund.

For this specific issue, I think keeping the change is as light as possible, i.e. my suggestion above. For ideal return types, we may create an issue for it, and much better have a clear RFC/guideline to avoid this kind of issue in the future.

[ismaeldcom]

Sure! I was thinking on the first cause of the issue, but for fixing it now, I'd stick with an early check 🙂

[htdat]

Added this commit 1c26bbd

I will take care of creating another issue later.

[htdat]

For this specific issue, I think keeping the change is as light as possible, i.e. my suggestion above. For ideal return types, we may create an issue for it, and much better have a clear RFC/guideline to avoid this kind of issue in the future.

FYI. Added the issue here #4293

[zmaglica]

I would move order fetching at the begging of the function, and use this if statement right after it to prevent getting all this data for nothing if the order is false

[htdat]

Updated the PR #4287 (comment)

Can someone give a final review before I merge it?

[ismaeldcom]

LGTM! Thanks for taking care of it 🙂