Totals Information Management - setting quote shipping method when address method is null

[joe-vortex]

Preconditions (*)

1. 2.3.2(2.3-develop) Magento Commerce or Open Source
2. Amasty Conditions module installed (cannot reproduce with vanilla m2 because this)
3. PHP 7.2.21

Steps to reproduce (*)

1. On the checkout, the component Amasty_Conditions/js/action/recollect-totals makes a call to /V1/guest-carts/:cartId/totals-information which passes through Magento\Checkout\Model\TotalsInformationManagement
2. In this model the following:

$quote->getShippingAddress()->setCollectShippingRates(true)->setShippingMethod(
     $addressInformation->getShippingCarrierCode() . '_' . $addressInformation->getShippingMethodCode()
);

is set, but when a shipping method has not been selected, it will return the quote shipping method with a total of 0 and a name of _, because $addressInformation->getShippingCarrierCode() and method code are null - so setShippingMethod('_') happens in effect.
3. This then means that validation for the shipping method is bypassed.
4. An order can then be placed without shipping information/carrier

Expected result (*)

1. If the address info has the shipping carrier and/or method code set to null, the setShippingMethod should not be called and the quote total is returned without setting a shipping code pair

Actual result (*)

1. When a address info instance is submitted with shipping codes set to null, a method is set with label _ and zero value, meaning an order can be placed without a valid shipping method.

[m2-assistant]

Hi @joe-vortex. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

• Summary of the issue
• Information on your environment
• Steps to reproduce
• Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.3-develop instance - upcoming 2.3.x release

For more details, please, review the Magento Contributor Assistant documentation.

@joe-vortex do you confirm that you were able to reproduce the issue on vanilla Magento instance following steps to reproduce?

• yes
• no

---

• Join Magento Community Engineering Slack and ask your questions in #github channel.

[joe-vortex]

can be fixed with the following patch:

--- magento/module-checkout/Model/TotalsInformationManagement.php	2019-10-18 13:17:40.275177439 +0100
+++ magento/module-checkout/Model/TotalsInformationManagement.php	2019-10-18 13:17:30.307000000 +0100
@@ -52,9 +52,11 @@
             $quote->setBillingAddress($addressInformation->getAddress());
         } else {
             $quote->setShippingAddress($addressInformation->getAddress());
-            $quote->getShippingAddress()->setCollectShippingRates(true)->setShippingMethod(
-                $addressInformation->getShippingCarrierCode() . '_' . $addressInformation->getShippingMethodCode()
-            );
+            if ($addressInformation->getShippingCarrierCode() && $addressInformation->getShippingMethodCode()) {
+                $quote->getShippingAddress()->setCollectShippingRates(true)->setShippingMethod(
+                    $addressInformation->getShippingCarrierCode().'_'.$addressInformation->getShippingMethodCode()
+                );
+            }
         }
         $quote->collectTotals();

[sdzhepa]

After discussion and investigation, I believe this issue is not specific only for Magento Commerce(EE) and the same problem should be in Open Source(CE) as well. It could be not possible to reproduce it by provided steps without buying the Amasty Conditions module but from a code perspective, we see the bug.

Confirmed

[magento-engcom-team]

✅ Confirmed by @sdzhepa
Thank you for verifying the issue. Based on the provided information internal tickets MC-22098 were created

Issue Available: @sdzhepa, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

[joe-vortex]

thanks @sdzhepa , are you happy for me to make a PR?

[m2-assistant]

Hi @joe-vortex. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

• 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

• 2. Verify that the issue is reproducible on 2.3-develop branch

  Details

  - Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

• 3. If the issue is not relevant or is not reproducible any more, feel free to close it.

---

• Join Magento Community Engineering Slack and ask your questions in #github channel.