Merge pull request #6867 from magento-cia/MC-41512

admanesachin · web-flow · commit c46efd26635d · 2021-05-16T11:21:53.000-05:00
[cia] MC-41512: [PSIRT-16607] PII leak of all purchases including guest ones due to an IDOR allowing the attacker to change any cart's customer id
diff --git a/app/code/Magento/Quote/Test/Mftf/Metadata/CustomerCartItemMeta.xml b/app/code/Magento/Quote/Test/Mftf/Metadata/CustomerCartItemMeta.xml
@@ -9,7 +9,7 @@
 <operations xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="urn:magento:mftf:DataGenerator/etc/dataOperation.xsd">
 
-    <operation name="CreateCustomerCartItem" dataType="CustomerCartItem" type="create" auth="adminOauth" url="/V1/carts/mine/items" method="POST">
+    <operation name="CreateCustomerCartItem" dataType="CustomerCartItem" type="create" auth="adminOauth" url="/V1/carts/{quote_id}/items" method="POST">
         <contentType>application/json</contentType>
         <object key="cartItem" dataType="CustomerCartItem">
             <field key="quote_id" type="string">string</field>
diff --git a/app/code/Magento/Quote/etc/webapi.xml b/app/code/Magento/Quote/etc/webapi.xml
@@ -65,7 +65,7 @@
             <resource ref="self" />
         </resources>
         <data>
-            <parameter name="cartId" force="true">%cart_id%</parameter>
+            <parameter name="quote.id" force="true">%cart_id%</parameter>
         </data>
     </route>
     <route url="/V1/carts/mine/order" method="PUT">
@@ -237,7 +237,7 @@
             <resource ref="self" />
         </resources>
         <data>
-            <parameter name="cartId" force="true">%cart_id%</parameter>
+            <parameter name="cartItem.quote_id" force="true">%cart_id%</parameter>
         </data>
     </route>
     <route url="/V1/carts/mine/items/:itemId" method="PUT">
@@ -246,7 +246,7 @@
             <resource ref="self" />
         </resources>
         <data>
-            <parameter name="cartId" force="true">%cart_id%</parameter>
+            <parameter name="cartItem.quote_id" force="true">%cart_id%</parameter>
         </data>
     </route>
     <route url="/V1/carts/mine/items/:itemId" method="DELETE">
diff --git a/app/code/Magento/Webapi/Controller/Rest/InputParamsResolver.php b/app/code/Magento/Webapi/Controller/Rest/InputParamsResolver.php
@@ -6,6 +6,10 @@
 
 namespace Magento\Webapi\Controller\Rest;
 
+use Magento\Framework\Api\SimpleDataObjectConverter;
+use Magento\Framework\App\ObjectManager;
+use Magento\Framework\Reflection\MethodsMap;
+use Magento\Framework\Webapi\Exception;
 use Magento\Framework\Webapi\ServiceInputProcessor;
 use Magento\Framework\Webapi\Rest\Request as RestRequest;
 use Magento\Webapi\Controller\Rest\Router\Route;
@@ -45,6 +49,11 @@ class InputParamsResolver
      */
     private $requestValidator;
 
+    /**
+     * @var MethodsMap
+     */
+    private $methodsMap;
+
     /**
      * Initialize dependencies
      *
@@ -53,26 +62,30 @@ class InputParamsResolver
      * @param ServiceInputProcessor $serviceInputProcessor
      * @param Router $router
      * @param RequestValidator $requestValidator
+     * @param MethodsMap|null $methodsMap
      */
     public function __construct(
         RestRequest $request,
         ParamsOverrider $paramsOverrider,
         ServiceInputProcessor $serviceInputProcessor,
         Router $router,
-        RequestValidator $requestValidator
+        RequestValidator $requestValidator,
+        MethodsMap $methodsMap = null
     ) {
         $this->request = $request;
         $this->paramsOverrider = $paramsOverrider;
         $this->serviceInputProcessor = $serviceInputProcessor;
         $this->router = $router;
         $this->requestValidator = $requestValidator;
+        $this->methodsMap = $methodsMap ?: ObjectManager::getInstance()
+            ->get(MethodsMap::class);
     }
 
     /**
      * Process and resolve input parameters
      *
      * @return array
-     * @throws \Magento\Framework\Webapi\Exception
+     * @throws Exception
      */
     public function resolve()
     {
@@ -81,6 +94,7 @@ public function resolve()
         $serviceMethodName = $route->getServiceMethod();
         $serviceClassName = $route->getServiceClass();
         $inputData = $this->getInputData();
+
         return $this->serviceInputProcessor->process($serviceClassName, $serviceMethodName, $inputData);
     }
 
@@ -108,6 +122,7 @@ public function getInputData()
         } else {
             $inputData = $this->request->getRequestData();
         }
+        $this->validateParameters($serviceClassName, $serviceMethodName, array_keys($route->getParameters()));
 
         return $this->paramsOverrider->override($inputData, $route->getParameters());
     }
@@ -122,6 +137,46 @@ public function getRoute()
         if (!$this->route) {
             $this->route = $this->router->match($this->request);
         }
+
         return $this->route;
     }
+
+    /**
+     * Validate that parameters are really used in the current request.
+     *
+     * @param string $serviceClassName
+     * @param string $serviceMethodName
+     * @param array $paramOverriders
+     */
+    private function validateParameters(
+        string $serviceClassName,
+        string $serviceMethodName,
+        array $paramOverriders
+    ): void {
+        $methodParams = $this->methodsMap->getMethodParams($serviceClassName, $serviceMethodName);
+        foreach ($paramOverriders as $key => $param) {
+            $arrayKeys = explode('.', $param);
+            $value = array_shift($arrayKeys);
+
+            foreach ($methodParams as $serviceMethodParam) {
+                $serviceMethodParamName = $serviceMethodParam[MethodsMap::METHOD_META_NAME];
+                $serviceMethodType = $serviceMethodParam[MethodsMap::METHOD_META_TYPE];
+
+                $camelCaseValue = SimpleDataObjectConverter::snakeCaseToCamelCase($value);
+                if ($serviceMethodParamName === $value || $serviceMethodParamName === $camelCaseValue) {
+                    if (count($arrayKeys) > 0) {
+                        $camelCaseKey = SimpleDataObjectConverter::snakeCaseToCamelCase('set_' . $arrayKeys[0]);
+                        $this->validateParameters($serviceMethodType, $camelCaseKey, [implode('.', $arrayKeys)]);
+                    }
+                    unset($paramOverriders[$key]);
+                    break;
+                }
+            }
+        }
+        if (!empty($paramOverriders)) {
+            throw new \UnexpectedValueException(
+                __('The current request does not expect the next parameters: ' . implode(', ', $paramOverriders))
+            );
+        }
+    }
 }
diff --git a/dev/tests/api-functional/_files/Magento/TestModule1/etc/webapi.xml b/dev/tests/api-functional/_files/Magento/TestModule1/etc/webapi.xml
@@ -72,6 +72,16 @@
             <resource ref="Magento_TestModule1::resource2" />
         </resources>
     </route>
+    <route method="PUT" url="/V1/testmodule1/withParam">
+        <service class="Magento\TestModule1\Service\V1\AllSoapAndRestInterface" method="update" />
+        <resources>
+            <resource ref="Magento_TestModule1::resource1" />
+            <resource ref="Magento_TestModule1::resource2" />
+        </resources>
+        <data>
+            <parameter name="paramId" force="true">%param_id%</parameter>
+        </data>
+    </route>
     <route method="GET" url="/V2/testmodule1/:id">
         <service class="Magento\TestModule1\Service\V2\AllSoapAndRestInterface" method="item" />
         <resources>
diff --git a/dev/tests/api-functional/testsuite/Magento/Webapi/Routing/CoreRoutingTest.php b/dev/tests/api-functional/testsuite/Magento/Webapi/Routing/CoreRoutingTest.php
@@ -9,18 +9,21 @@
  */
 namespace Magento\Webapi\Routing;
 
+use Magento\Framework\Webapi\Rest\Request;
+use Magento\Integration\Model\Integration;
+use Magento\TestFramework\Authentication\OauthHelper;
 use Magento\TestFramework\Helper\Bootstrap;
 use Magento\TestFramework\TestCase\Webapi\Adapter\Rest\RestClient;
 
-class CoreRoutingTest extends \Magento\Webapi\Routing\BaseService
+class CoreRoutingTest extends BaseService
 {
     public function testBasicRoutingExplicitPath()
     {
         $itemId = 1;
         $serviceInfo = [
             'rest' => [
                 'resourcePath' => '/V1/testmodule1/' . $itemId,
-                'httpMethod' => \Magento\Framework\Webapi\Rest\Request::HTTP_METHOD_GET,
+                'httpMethod' => Request::HTTP_METHOD_GET,
             ],
             'soap' => [
                 'service' => 'testModule1AllSoapAndRestV1',
@@ -38,7 +41,7 @@ public function testDisabledIntegrationAuthorizationException()
         $serviceInfo = [
             'rest' => [
                 'resourcePath' => '/V1/testmodule1/' . $itemId,
-                'httpMethod' => \Magento\Framework\Webapi\Rest\Request::HTTP_METHOD_GET,
+                'httpMethod' => Request::HTTP_METHOD_GET,
             ],
             'soap' => [
                 'service' => 'testModule1AllSoapAndRestV1',
@@ -48,11 +51,11 @@ public function testDisabledIntegrationAuthorizationException()
         $requestData = ['itemId' => $itemId];
 
         /** Disable integration associated with active OAuth credentials. */
-        $credentials = \Magento\TestFramework\Authentication\OauthHelper::getApiAccessCredentials();
-        /** @var \Magento\Integration\Model\Integration $integration */
+        $credentials = OauthHelper::getApiAccessCredentials();
+        /** @var Integration $integration */
         $integration = $credentials['integration'];
         $originalStatus = $integration->getStatus();
-        $integration->setStatus(\Magento\Integration\Model\Integration::STATUS_INACTIVE)->save();
+        $integration->setStatus(Integration::STATUS_INACTIVE)->save();
 
         try {
             $this->assertUnauthorizedException($serviceInfo, $requestData);
@@ -83,9 +86,37 @@ public function testRestNoAcceptHeader()
         $this->_markTestAsRestOnly();
         /** @var $curlClient RestClient */
         $curlClient = Bootstrap::getObjectManager()->get(
-            \Magento\TestFramework\TestCase\Webapi\Adapter\Rest\RestClient::class
+            RestClient::class
         );
         $response = $curlClient->get('/V1/testmodule1/resource1/1', [], ['Accept:']);
         $this->assertEquals('testProduct1', $response['name'], "Empty Accept header failed to return response.");
     }
+
+    /**
+     * Verifies that exception is thrown when the request contains unexpected parameters.
+     *
+     * @return void
+     */
+    public function testRequestParamsUnexpectedValueException(): void
+    {
+        $this->_markTestAsRestOnly();
+        $expectedMessage = "Internal Error. Details are available in Magento log file. Report ID: webapi-";
+        $unexpectedMessage = "\"%fieldName\" is required. Enter and try again.";
+
+        $serviceInfo = [
+            'rest' => [
+                'resourcePath' => '/V1/testmodule1/withParam',
+                'httpMethod' => Request::HTTP_METHOD_PUT,
+            ],
+        ];
+
+        try {
+            $this->_webApiCall($serviceInfo);
+        } catch (\Exception $e) {
+            $exceptionResult = $this->processRestExceptionResult($e);
+            $actualMessage = $exceptionResult['message'];
+            $this->assertStringNotContainsString($unexpectedMessage, $actualMessage);
+            $this->assertStringContainsString($expectedMessage, $actualMessage);
+        }
+    }
 }
diff --git a/dev/tests/integration/testsuite/Magento/Checkout/_files/quote_with_address_another_customer.php b/dev/tests/integration/testsuite/Magento/Checkout/_files/quote_with_address_another_customer.php
@@ -0,0 +1,52 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+use Magento\Catalog\Api\ProductRepositoryInterface;
+use Magento\Customer\Api\AccountManagementInterface;
+use Magento\Customer\Api\CustomerRepositoryInterface;
+use Magento\Quote\Api\CartRepositoryInterface;
+use Magento\Quote\Model\Quote\AddressFactory;
+use Magento\Quote\Model\QuoteFactory;
+use Magento\Store\Api\WebsiteRepositoryInterface;
+use Magento\TestFramework\Helper\Bootstrap;
+use Magento\TestFramework\Workaround\Override\Fixture\Resolver;
+
+Resolver::getInstance()->requireDataFixture('Magento/Customer/_files/customer_with_addresses.php');
+Resolver::getInstance()->requireDataFixture('Magento/Catalog/_files/products.php');
+
+$objectManager = Bootstrap::getObjectManager();
+
+/** Import Customer Address Data */
+$quoteShippingAddress = $objectManager->get(AddressFactory::class)->create();
+$customerRepository = $objectManager->get(CustomerRepositoryInterface::class);
+$customer = $customerRepository->get('customer_with_addresses@test.com');
+$addresses = $customer->getAddresses();
+$addressFirst = array_shift($addresses);
+$quoteShippingAddress->importCustomerAddressData($addressFirst);
+
+$productRepository = $objectManager->create(ProductRepositoryInterface::class);
+$product = $productRepository->get('simple');
+
+$websiteRepository = $objectManager->get(WebsiteRepositoryInterface::class);
+$mainWebsite = $websiteRepository->get('base');
+
+$accountManagement = $objectManager->create(AccountManagementInterface::class);
+
+$quoteRepository = $objectManager->get(CartRepositoryInterface::class);
+$quote = $objectManager->get(QuoteFactory::class)->create();
+$quote->setStoreId($mainWebsite->getDefaultStore()->getId())
+    ->setIsActive(true)
+    ->setIsMultiShipping(false)
+    ->assignCustomerWithAddressChange($customer)
+    ->setShippingAddress($quoteShippingAddress)
+    ->setBillingAddress($quoteShippingAddress)
+    ->setCheckoutMethod('customer')
+    ->setPasswordHash($accountManagement->getPasswordHash('password'))
+    ->setReservedOrderId('test_order_999')
+    ->setCustomerEmail('aaa2@aaa.com')
+    ->addProduct($product, 2);
+$quoteRepository->save($quote);
diff --git a/dev/tests/integration/testsuite/Magento/Checkout/_files/quote_with_address_another_customer_rollback.php b/dev/tests/integration/testsuite/Magento/Checkout/_files/quote_with_address_another_customer_rollback.php
@@ -0,0 +1,24 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+declare(strict_types=1);
+
+use Magento\Framework\Api\SearchCriteriaBuilder;
+use Magento\Quote\Api\CartRepositoryInterface;
+use Magento\TestFramework\Helper\Bootstrap;
+use Magento\TestFramework\Workaround\Override\Fixture\Resolver;
+
+Resolver::getInstance()->requireDataFixture('Magento/Customer/_files/customer_with_addresses_rollback.php');
+Resolver::getInstance()->requireDataFixture('Magento/Catalog/_files/products_rollback.php');
+
+$objectManager = Bootstrap::getObjectManager();
+$searchCriteriaBuilder = $objectManager->get(SearchCriteriaBuilder::class);
+$searchCriteria = $searchCriteriaBuilder->addFilter('reserved_order_id', 'test_order_999')->create();
+
+/** @var CartRepositoryInterface $quoteRepository */
+$quoteRepository = $objectManager->get(CartRepositoryInterface::class);
+$items = $quoteRepository->getList($searchCriteria)->getItems();
+$item = array_pop($items);
+$quoteRepository->delete($item);
