Prevent orders being placed when no shipping options are available #46026

opr · 2024-03-27T22:02:06Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Adds a check to the OrderController's validate_selected_shipping_methods method to check if the selected shipping method is a valid method.

In https://github.com/woocommerce/woocommerce/pull/43869/files#diff-cdc9f1829f52d7b83e3542948622d924ef5cf6a3e0b4580e6ebd1c9643bc49b4R501 we made some changes to shipping method handling to prevent fatal errors, the chosen method was cast to a string in this PR.

This meant our check in validate_selected_shipping_methods no longer worked because we are checking for: if ( false === $chosen_shipping_method ) which would no longer be the case.

In this PR I am validating a little further by checking the selected method against the available methods.

We set the default value of the chosen_shipping_method session variable to [] when validating orders. This means if it's unset, and shipping is required, sessions with no value will not be allowed to sneak through.

This PR brings some changes to PHP unit tests. In the Checkout route unit tests, we now add a Flat Rate shipping method before each test (and remove it afterwards). This is because previously we were using the legacy flat rate which didn't need to be part of a zone to work. This meant that it was not possible to test the fix for this bug as it requires a zone to have a (disabled) shipping method.

We also discovered an issue with the test_checkout_force_create_account, test_checkout_do_not_create_account, and test_checkout_create_account tests. They were passing even though no valid shipping method was selected. This PR fixes that by introducing a new class to override WC()->session called. MockSessionHandler.

In MockSessionHandler I changed places where wp_set_cache was used and implemented a class property to act as the cache, and also overrode the set function to automatically update the cache when setting data. This was required as the $this->_data was overwritten with blank data from the empty cache when logging in as a user. The session was never saved to the DB as the PHPUnit tests run in a single context, so by the time shutdown runs the test is over.

In these tests that use it, we are explicitly overriding WC()->session as there is no good way of setting this up using the woocommerce_session_handler filter on a per-test basis.

These tests need special handling as they need to be able to set a cookie during guest -> logged in user switching.

Closes #45912.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Have a shipping zone with at least one shipping method, but do not enable it. (If there are no shipping zones/methods at all, Woo will not include a shipping section at all on the checkout page.)
Make sure that the checkout page is built with the Checkout Block.
As a customer, buy a product and go to the checkout page.
Eenter an address that does not support shipping. You'll want to see the message "There are no shipping options available. Please check your shipping address."
Select the Place Order button.
Ensure the order does not place.

Regression testing

Enable various shipping zones and various methods (across different countries).
Place an order with them all and ensure no errors are encountered.

Changelog entry

Automatically create a changelog entry from the details below.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

Comment

github-actions · 2024-03-27T22:02:37Z

Hi @senadir,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

senadir

I think this is in the right direction, I do think we should be more defensive in validate_order_before_payment.

For the test, I left some comments but overall I think it's fine. We might need to refactor it in the future to ensure it's as close as possible to real life session manager to avoid false positives.

senadir · 2024-03-29T11:47:16Z

plugins/woocommerce/src/StoreApi/Utilities/OrderController.php

@@ -168,7 +168,7 @@ public function sync_customer_data_with_order( \WC_Order $order ) {
 	 */
 	public function validate_order_before_payment( \WC_Order $order ) {
 		$needs_shipping          = wc()->cart->needs_shipping();
-		$chosen_shipping_methods = wc()->session->get( 'chosen_shipping_methods' );
+		$chosen_shipping_methods = wc()->session->get( 'chosen_shipping_methods', [ '' ] );


I think it's best to have this as an empty array and strengthen the checks in validate_selected_shipping_methods by separating the check for need_shipping out, and doing array validation second.

senadir · 2024-03-29T11:49:03Z

plugins/woocommerce/tests/php/src/Blocks/StoreApi/MockSessionHandler.php

+	public function init_session_cookie( $set = true ) {
+		$to_hash           = $this->_customer_id . '|' . $this->_session_expiration;
+		$cookie_hash       = hash_hmac( 'md5', $to_hash, wp_hash( $to_hash ) );
+		$cookie_value      = $this->_customer_id . '||' . $this->_session_expiration . '||' . $this->_session_expiring . '||' . $cookie_hash;
+		$this->_has_cookie = true;
+
+		if ( ! isset( $_COOKIE[ $this->_cookie ] ) || $_COOKIE[ $this->_cookie ] !== $cookie_value ) {
+
+			// Manually set the cookie here because the response is already sent, so wc_setcookie() (as in the parent class) won't work.
+			$_COOKIE[ $this->_cookie ] = $cookie_value;
+		}
+	}
+}


This might be a bit risky because it's overreaching, in this case you're replacing init_session_cookie but are you handling the process of moving from guest to logged in?

Refactored now. The functions are the same as the WC_Session_Handler except instead of using wp_cache it uses a mock cache. For some reason caching was preventing the switch from logged out to logged in customer from working.

During this switch, the session data is saved in cache and re-read later when setting the session up for a logged-in user. In tests the cache was not saving it.

senadir · 2024-03-29T11:50:02Z

plugins/woocommerce/tests/php/src/Blocks/StoreApi/Routes/Checkout.php

+		// We need to replace the WC_Session with a mock because this test relies on cookies being set which
+		// is not easy with PHPUnit. This is a simpler approach.
+		$old_session  = WC()->session;
+		WC()->session = new MockSessionHandler();


I guess the woocommerce_session_handler filter didn't work?

Yep, afraid not the only place to set this up was in plugins/woocommerce/tests/legacy/bootstrap.php which would affect all tests, not just the ones we need it for. This is a good solution for now.

senadir

Working fine! thank you for doing the changes.

senadir

Thanks for the changes!

We might need to look into the future of ways to reduce the size of the session handler.

…46026) * Prevent orders being placed with invalid shipping options * Add changelog * Add shipping_disable_flat_rate fixture function * Test checking out with no valid shipping methods selected * Update tests to add a default shipping method * Update test_checkout_invalid_shipping_method to disable method * If shipping methods is null, return an array with an empty string inside * Replace WC session in tests that rely on setting cookies * Add MockSessionHandler to handle test cases using cookies * Add docblock comment * Expect shipping validation to fail if chosen methods are null * Add shipping method before testing validate_selected_shipping_methods * Update MockSessionHandler to handle caching * Show error when test fails * Default the chosen shipping methods to an empty array if not set * Split checks for needs_shipping and valid shipping apart * Remove unnecessary session set and total calculation * Fix lint errors * Init session in each test * Reimplement required methods (those that are private or use cookies) * Update phpcs ignore comment to be inline * Prevent error when accessing unset variable in mock cache * Fix lint error

* Prevent orders being placed when no shipping options are available (#46026) * Prevent orders being placed with invalid shipping options * Add changelog * Add shipping_disable_flat_rate fixture function * Test checking out with no valid shipping methods selected * Update tests to add a default shipping method * Update test_checkout_invalid_shipping_method to disable method * If shipping methods is null, return an array with an empty string inside * Replace WC session in tests that rely on setting cookies * Add MockSessionHandler to handle test cases using cookies * Add docblock comment * Expect shipping validation to fail if chosen methods are null * Add shipping method before testing validate_selected_shipping_methods * Update MockSessionHandler to handle caching * Show error when test fails * Default the chosen shipping methods to an empty array if not set * Split checks for needs_shipping and valid shipping apart * Remove unnecessary session set and total calculation * Fix lint errors * Init session in each test * Reimplement required methods (those that are private or use cookies) * Update phpcs ignore comment to be inline * Prevent error when accessing unset variable in mock cache * Fix lint error * Prep for cherry pick 46026 --------- Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com> Co-authored-by: WooCommerce Bot <no-reply@woo.com>

…46026) * Prevent orders being placed with invalid shipping options * Add changelog * Add shipping_disable_flat_rate fixture function * Test checking out with no valid shipping methods selected * Update tests to add a default shipping method * Update test_checkout_invalid_shipping_method to disable method * If shipping methods is null, return an array with an empty string inside * Replace WC session in tests that rely on setting cookies * Add MockSessionHandler to handle test cases using cookies * Add docblock comment * Expect shipping validation to fail if chosen methods are null * Add shipping method before testing validate_selected_shipping_methods * Update MockSessionHandler to handle caching * Show error when test fails * Default the chosen shipping methods to an empty array if not set * Split checks for needs_shipping and valid shipping apart * Remove unnecessary session set and total calculation * Fix lint errors * Init session in each test * Reimplement required methods (those that are private or use cookies) * Update phpcs ignore comment to be inline * Prevent error when accessing unset variable in mock cache * Fix lint error

opr added 2 commits March 27, 2024 22:01

Prevent orders being placed with invalid shipping options

fabd2b4

Add changelog

d197e06

opr added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 27, 2024

opr self-assigned this Mar 27, 2024

woocommercebot requested review from a team and senadir and removed request for a team March 27, 2024 22:02

opr added 2 commits March 27, 2024 23:24

Add shipping_disable_flat_rate fixture function

e218a57

Test checking out with no valid shipping methods selected

986193e

opr marked this pull request as draft March 28, 2024 08:05

Stojdza added this to the 8.8.0 milestone Mar 28, 2024

github-actions bot removed this from the 8.8.0 milestone Mar 28, 2024

opr added 8 commits March 28, 2024 19:40

Update tests to add a default shipping method

49bb0a1

Update test_checkout_invalid_shipping_method to disable method

54c0146

If shipping methods is null, return an array with an empty string inside

6b65384

Replace WC session in tests that rely on setting cookies

4a9fe6a

Add MockSessionHandler to handle test cases using cookies

28faf96

Add docblock comment

a81339d

Expect shipping validation to fail if chosen methods are null

59256e6

Add shipping method before testing validate_selected_shipping_methods

c142289

opr marked this pull request as ready for review March 28, 2024 21:39

woocommercebot requested a review from a team March 28, 2024 21:39

senadir requested changes Mar 29, 2024

View reviewed changes

opr added 5 commits March 29, 2024 15:33

Update MockSessionHandler to handle caching

8ff4b0d

Show error when test fails

63b93c9

Default the chosen shipping methods to an empty array if not set

ad0e34e

Split checks for needs_shipping and valid shipping apart

cb05017

Remove unnecessary session set and total calculation

594b553

opr requested a review from senadir March 29, 2024 15:46

senadir approved these changes Mar 29, 2024

View reviewed changes

opr added 6 commits March 29, 2024 15:56

Fix lint errors

567379a

Init session in each test

c8421ae

Reimplement required methods (those that are private or use cookies)

62ba81b

Update phpcs ignore comment to be inline

b5cf360

Prevent error when accessing unset variable in mock cache

2d4be70

Fix lint error

80e5929

opr requested a review from senadir March 29, 2024 21:36

senadir approved these changes Apr 2, 2024

View reviewed changes

senadir merged commit 68c6ec6 into trunk Apr 2, 2024
27 checks passed

senadir deleted the fix/45912-shipping-options branch April 2, 2024 09:03

github-actions bot added this to the 8.9.0 milestone Apr 2, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Apr 2, 2024

Stojdza modified the milestones: 8.9.0, 8.8.0 Apr 3, 2024

This was referenced Apr 3, 2024

Cherry pick 46026 into release/8.8 #46171

Merged

Delete changelog files based on PR 46026 #46172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent orders being placed when no shipping options are available #46026

Prevent orders being placed when no shipping options are available #46026

opr commented Mar 27, 2024 •

edited

github-actions bot commented Mar 27, 2024 •

edited

senadir left a comment

senadir Mar 29, 2024

opr Mar 29, 2024

senadir Mar 29, 2024

opr Mar 29, 2024

senadir Mar 29, 2024

opr Mar 29, 2024

senadir left a comment

senadir left a comment

Prevent orders being placed when no shipping options are available #46026

Prevent orders being placed when no shipping options are available #46026

Conversation

opr commented Mar 27, 2024 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Regression testing

Changelog entry

Significance

Type

Message

Comment

github-actions bot commented Mar 27, 2024 • edited

senadir left a comment

Choose a reason for hiding this comment

senadir Mar 29, 2024

Choose a reason for hiding this comment

opr Mar 29, 2024

Choose a reason for hiding this comment

senadir Mar 29, 2024

Choose a reason for hiding this comment

opr Mar 29, 2024

Choose a reason for hiding this comment

senadir Mar 29, 2024

Choose a reason for hiding this comment

opr Mar 29, 2024

Choose a reason for hiding this comment

senadir left a comment

Choose a reason for hiding this comment

senadir left a comment

Choose a reason for hiding this comment

opr commented Mar 27, 2024 •

edited

github-actions bot commented Mar 27, 2024 •

edited