magento icon indicating copy to clipboard operation
magento copied to clipboard

Memory leak and probably sql code injection rest api order search calls "/rest/V1/orders"

Open govereem opened this issue 4 years ago • 1 comments

https://github.com/myparcelnl/magento/blame/master/Plugin/Magento/Sales/Api/Data/OrderExtension.php

        $sql = $connection
            ->select('myparcel_delivery_options')
            ->from($tableName)
            ->where($searchColumn . ' = ' . (int) $searchValue);

        $result = $connection->fetchAll($sql); 

When rest searching "/rest/V1/orders" with order incremented_id (string value), the (int) type cating will make the string value to 0, leaving us with a sql command select * from sales_order where incremented_id = 0. When doing this for 250 records gives a big memory overhead. Also the select('myparcel_delivery_options') should be select(). And from($tableName) should be from($tableName, 'myparcel_delivery_options'). Why probably sql code injection? this is because the databinding is missing [':searchValue' => $searchValue].

If I look at this solution is, it looks like a hacky solution, the extension_attributes.xml should solve this, but somehow its not working, and this solution was a work-around. Maybe it has something todo with this interface not beying correct vendor/myparcelnl/magento/Api/Data/DeliveryOptions.php. But a better work-around would be something like a plugin on Magento\Sales\Api\OrderRepositoryInterface afterGetlist.

By example:

<?php

namespace ....\ExtendMyParcel\Plugin;

use Magento\Sales\Api\Data\OrderExtensionInterface;
use Magento\Sales\Api\Data\OrderInterface;
use Magento\Sales\Api\Data\OrderSearchResultInterface;
use Magento\Sales\Api\Data\OrderExtensionFactory;
use ....\ExtendMyParcel\Model\DeliveryOptionsFactory;

class OrderRepository
{
    const FIELD = 'myparcel_delivery_options';
    /**
     * @var OrderExtensionFactory
     */
    private $orderExtensionFactory;
    /**
     * @var DeliveryOptionsFactory
     */
    private $deliveryOptionsFactory;

    /**
     * OrderRepository constructor.
     * @param OrderExtensionFactory $orderExtensionFactory
     */
    public function __construct(
        OrderExtensionFactory $orderExtensionFactory,
        DeliveryOptionsFactory $deliveryOptionsFactory
    )
    {
        $this->orderExtensionFactory = $orderExtensionFactory;
        $this->deliveryOptionsFactory = $deliveryOptionsFactory;
    }

    public function afterGetList($subject, $searchResult)
    {
        if (!$searchResult instanceof OrderSearchResultInterface) {
            return $searchResult;
        }

        foreach ($searchResult->getItems() as $order) {
            $this->setMyParcel($order);
        }

        return $searchResult;
    }

    public function setMyParcel(OrderInterface $order)
    {
        /** @var OrderExtensionInterface $extensionAttributes */
        $extensionAttributes = $order->getExtensionAttributes();

        if ($extensionAttributes === null) {
            $extensionAttributes = $this->orderExtensionFactory->create();
        } elseif ($extensionAttributes->getMyparcelDeliveryOptions() !== null) {
            return;
        }

        $deliveryOptions = $order->getData('myparcel_delivery_options') ?? "";

        /** @var \Postdrogist\ExtendMyParcel\Model\DeliveryOptions $deliveryOptionsItem */
        $deliveryOptionsItem = $this->deliveryOptionsFactory->create();
        $deliveryOptionsItem->setValue($deliveryOptions);
        $extensionAttributes->setMyparcelDeliveryOptions($deliveryOptionsItem);
        $order->setExtensionAttributes($extensionAttributes);
    }
}

govereem avatar Jan 14 '21 09:01 govereem